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

fix: ordering-rule support for top level statements #393

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

dbale-altoros
Copy link
Collaborator

This is the #377 with unit tests addition to check the behavior

credits to @pahor167

@fvictorio
Copy link
Contributor

Custom errors can also be at the file-level (that is, outside a contract) and this PR doesn't handle that.

@dbale-altoros dbale-altoros marked this pull request as draft February 7, 2023 13:27
@dbale-altoros dbale-altoros changed the title fix: ordering-rule custom error support DRAFT: fix: ordering-rule custom error support Feb 7, 2023
@dbale-altoros
Copy link
Collaborator Author

Custom errors can also be at the file-level (that is, outside a contract) and this PR doesn't handle that.

@fvictorio
good catch!
does this rule includes free functions and constant variables (file level) ?

@fvictorio
Copy link
Contributor

I don't remember, check the implementation.

@dbale-altoros
Copy link
Collaborator Author

Added support for:

At File Level:

  • custom errors
  • constants
  • free functions

At Contract Level:

  • custom errors

@dbale-altoros dbale-altoros marked this pull request as ready for review February 14, 2023 21:28
@dbale-altoros dbale-altoros changed the title DRAFT: fix: ordering-rule custom error support fix: ordering-rule custom error support Feb 14, 2023
uint256 constant oneNiceConstant = 1;
function freeFunction() pure returns (uint256) {
return 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@frangio sorry for the ping, but do you have an opinion on this order for file-level nodes? The new supported elements are custom errors, constants and functions.

I feel like I would swap constants and errors here, so that the order is:

  • Constants
  • Enum/Structs
  • Custom errors
  • Functions
  • Contracts

Bikeshedding though.

Copy link

Choose a reason for hiding this comment

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

No opinion here to be honest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fvictorio I feel the order you proposed is a better one
I changed the weight so it can match what you suggested
Please approve when you have time

@dbale-altoros dbale-altoros merged commit d97d0fb into master Feb 22, 2023
@dbale-altoros dbale-altoros deleted the fix/ordering-rule-for-custom-errors branch February 22, 2023 13:32
@dbale-altoros dbale-altoros changed the title fix: ordering-rule custom error support fix: ordering-rule support for top level statements Mar 6, 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.

4 participants