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

Format MASM #1194

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Format MASM #1194

merged 1 commit into from
Mar 3, 2025

Conversation

partylikeits1983
Copy link
Contributor

I ran my Miden Assembly code formatter on the crates/miden-lib/asm directory and discovered several formatting issues:

  • Trailing white space at the end of lines
  • Incorrectly indented comments
  • Consecutive empty lines

You can install the masm formatter from crates.io using:

cargo install masm-formatter

To run the formatter on a specific directory (e.g., crates/miden-lib/asm), execute:

cargo masm-fmt crates/miden-lib/asm

This PR applies the formatter's changes, addressing the issues noted above.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

Does the formatter support --check, i.e. checking if things are formatted? If so, should we add a CI lint maybe?

@partylikeits1983
Copy link
Contributor Author

Does the formatter support --check, i.e. checking if things are formatted? If so, should we add a CI lint maybe?

I can add the --check flag. I would like to maybe get a review of the masm-formatter before adding it to the CI. Maybe we can make it a Miden repository so that I can't accidentally push something to crates and then make all CI checks fail 😅

@PhilippGackstatter
Copy link
Contributor

I can add the --check flag. I would like to maybe get a review of the masm-formatter before adding it to the CI. Maybe we can make it a Miden repository so that I can't accidentally push something to crates and then make all CI checks fail 😅

Yeah, on second thought, test-driving it for a bit is probably a good idea, as well as moving it under the org.

@partylikeits1983
Copy link
Contributor Author

@PhilippGackstatter Does this PR need a changelog?

@bobbinth bobbinth added the no changelog This PR does not require an entry in the `CHANGELOG.md` file label Mar 3, 2025
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@bobbinth bobbinth merged commit 1472d0f into 0xPolygonMiden:next Mar 3, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This PR does not require an entry in the `CHANGELOG.md` file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants