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.

Sign up for GitHub

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

Jump to bottom

Try foundry fmt #136

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Try foundry fmt #136

wants to merge 2 commits into from

Conversation

NIC619
Copy link
Contributor

@NIC619 NIC619 commented Jan 10, 2023

No description provided.

@NIC619 NIC619 marked this pull request as draft January 10, 2023 07:38
NIC619
NIC619 commented Jan 10, 2023
View reviewed changes
contracts/interfaces/ILimitOrder.sol
bytes32 allowFillHash,
address recipient,
FillReceipt fillReceipt
bytes32 indexed orderHash, address indexed maker, address indexed taker, bytes32 allowFillHash, address recipient, FillReceipt fillReceipt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This creates different result from prettier and fmt: prettier has a problem but fmt does not, though this line does not exceed 160 characters...so I'm not sure why prettier is complaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this difference from fmt results in it updating many function and event declarations. This one is one of the many examples.

@NIC619
Copy link
Contributor Author

NIC619 commented Jan 12, 2023

prettier will convert into multiline if number of function param exceed three, even if line length hasn't reach 160 limit.

@pilagod
Copy link
Contributor

pilagod commented Jan 13, 2023

Weird unwritten rules 😅

Overall I think prettier is a better fit to our preference for now. We can delay the foundry fmt integration and keep tracking its updates until it is better enough 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pilagod pilagod Awaiting requested review from pilagod

@charlesjhongc charlesjhongc Awaiting requested review from charlesjhongc

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NIC619 @pilagod