-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(forge fmt): Adds tab support as indent char in fmt #10979
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
base: master
Are you sure you want to change the base?
Conversation
thank you! looks like it breaks tests for win |
Thanks for running the tests. Fixing this will take a while for me because there seems to be a bug with newlines. With CRLF it still adds indents ( function test4() public {
// forgefmt: disable-next-line
assembly{
sstore(1, 1)
sstore(2, 2)
sstore(3, 3)// forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
if (condition) execute(); // comment7
} While with LF line endings it doesn't ( function test4() public {
// forgefmt: disable-next-line
assembly{
sstore(1, 1)
sstore(2, 2)
sstore(3, 3)// forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
if (condition) execute(); // comment7
} I assume there is a bug in handling |
@grandizzy I think I found and fixed the issue with the last commit. Tests succeed on my fork. |
@grandizzy I think I fixed the bug, but other tests are now failing in parts that I haven't touched. Could this be a race condition or timeouts that happen in tests? |
hey @mathewmeconry yeah, we're debugging some unexpected failures in CI, will get back to the pr once figured out. Thank you |
Motivation
I want to start to contribute and the issue #10754 was marked as a good first issue.
Solution
Adds support for different styles in fmt config. This allows the user to configure whether tabs or spaces should be used (I don't want to start the age-old discussion here).
tab_width
will be ignored by fmt if the style is set totab
.If no style is given in foundry.toml
space
will be used as default.Coverting block comments from space to tab results in issues where the previous spaces will not be ignored. I couldn't figure out a way to solve this properly but I believe it is a non-issue because most teams won't switch from spaces to tabs that often and fixing some comments is fine from my point of view.
Additional to the tab support, a bug in the parsing in
InlineConfig
forforgefmt: disable-line
handling was found and fixed. The previous code doesn't handle CRLF line endings properly and returns the wrong loc for the disabled line (offset by 1 byte).PR Checklist