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

Update Prettier Solidity #3898

Merged
merged 5 commits into from
Dec 24, 2022
Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Dec 23, 2022

The new version of Prettier Solidity changes a few things about how it formats the code, so this PR includes many small changes to formatting.

I wrote a little program to check that files in this PR didn't change in their actual Solidity content, other than comments and whitespace: solidity-fingerprint.

Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Ran tests for both master and this branch.
Everything seems fine 👍

GIst

CHANGELOG.md Show resolved Hide resolved
@frangio frangio merged commit b709eae into OpenZeppelin:master Dec 24, 2022
@frangio frangio deleted the update-prettier branch December 24, 2022 01:28
@Janther
Copy link
Contributor

Janther commented Dec 30, 2022

there are a few scenarios where prettier-plugin-solidity changes the code to improve readability. However we make sure these changes don't affect the compiled code.
For example a * b / c will be formatted as (a * b) / c or a ** b ** c will result in a ** (b ** c).
these changes will generate a different fingerprint in your program since we added () but the compiled code will be the same.
So in the future, when we release a new version, you might want to consider comparing compiled codes instead.

@frangio
Copy link
Contributor Author

frangio commented Dec 30, 2022

@Janther The issue is that things like abstract contracts don't produce bytecode. But you're right that the fingerprint as I defined it here is not the right approach. Instead of computing a fingerprint of the token stream, it should probably be of the AST.

frangio added a commit that referenced this pull request Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants