Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: forc-index-build preserve yaml comments #1415

Closed
wants to merge 2 commits into from

Conversation

ra0x3
Copy link
Contributor

@ra0x3 ra0x3 commented Oct 16, 2023

Thanks for opening a PR with the Fuel Indexer project. Please review the Checklist below and ensure you've completed all of the necessary steps to make this PR review as painless as possible.

Checklist

  • Ensure your top-level commit message is in line with our contributor guidelines.
  • Please add proper labels.
  • If there is an issue associated with this PR, please link the issue (right-hand sidebar)
  • If there is not an issue associated with this PR, add this PR to the "Fuel Indexer" project (right-hand sidebar)
  • Please allow Codeowners at least 24 hours to do a first-pass review.
  • Please add thoroughly detailed testing steps below.
  • Please keep your Changelog message short and sweet.

Description

  • PR makes it such that comments will remain in your manifest after forc index build
  • serde_yaml obviously does not (and will not) handle comments
  • The idea in this PR is to just add a small manifest proxy RawManifest to cache comments on read, and write the comments back on write
  • This was actually not easy 😅

Testing steps

  • Build plugin binary locally cargo build -p forc-index --release
  • Create a new project using local binary /path/to/fuel-indexer/target/release/forc-index new fool --namespace fuel
  • Build it /path/to/fuel-indexer/target/release/forc-index build
  • You should notice that the comments remain in the same place after forc index build

Changelog

  • chore: forc-index-build preserve yaml comments

@ra0x3 ra0x3 linked an issue Oct 16, 2023 that may be closed by this pull request
@ra0x3 ra0x3 self-assigned this Oct 16, 2023
@ra0x3 ra0x3 force-pushed the rashad/1350-preserve-manifest-comments branch from 8a36af1 to 1f3981a Compare October 17, 2023 15:19
@ra0x3 ra0x3 force-pushed the rashad/1350-preserve-manifest-comments branch from 1f3981a to 1f8a332 Compare October 17, 2023 16:45
@ra0x3 ra0x3 marked this pull request as ready for review October 17, 2023 19:43
@@ -32,6 +32,7 @@ graphql_schema: examples/fuel-explorer/fuel-explorer/schema/fuel_explorer.schem
# of the indexer.
# Important: At this time, wasm is the preferred method of execution.
module:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add an empty line here? Is this something the comment-preserving code is forcing us to do?

Copy link
Contributor Author

@ra0x3 ra0x3 Oct 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lostman

  • Well the alternative to simply joining each line in the manifest on a newline, is - finding the particular lines that (we think) shouldn't have new lines above them (e.g., wasm: ~) and removing the newline in-between this line and its precursor line
    • Nothing forcing us to not do this, I just thought it was a little much for something small/subjective

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I think that's fine. What I expected is that nested items would not be separated by empty lines:

toplevel:
  nested: 123

another-top-level:
  another-nested: 123

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lostman Let me push up a change

@ra0x3 ra0x3 marked this pull request as draft October 18, 2023 15:24
@ra0x3 ra0x3 closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forc index build should preserve comments in manifest
2 participants