-
Notifications
You must be signed in to change notification settings - Fork 2.2k
chore(fmt): rename and alias params config varaint for clarity #11944
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, let's update README too https://github.com/foundry-rs/foundry/tree/master/crates/fmt#configuration and dupe the test for params_first
too to make sure alias does not get removed / introduce breaking change
i tested both locally to make sure. do we still want to duplicate the whole file just for that? completely forgot about the readme, thanks for the reminder 🤝 |
oh, not the whole file, but maybe just a test with inline config to make sure we don't remove the alias in future without ack the breaking change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Motivation
the naming for the the
multiline_func_header
config variant"params_first"
is counter intuitive, as it is not aligned with the behavior of"params_first_multi"
.see how it created confusion for users, who opened this issue:
multiline_func_header = "params_first"
seems to always break the params #11933Solution
because of that, i renamed the config variant to
"params_always"
to better reflect that this styling option will ALWAYS break the fn header params.the old name
"params_first"
is kept as an alias for backwards compatibilityPR Checklist