Skip to content

Update Prettier Solidity #3898

Merged
frangio merged 5 commits into
OpenZeppelin:masterfrom
frangio:update-prettier
Dec 24, 2022
Merged

Update Prettier Solidity #3898
frangio merged 5 commits into
OpenZeppelin:masterfrom
frangio:update-prettier

Conversation

@frangio
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread CHANGELOG.md
@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
Copy Markdown
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
Copy Markdown
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