-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fixed SwayFmt removing comments in configurable blocks #5297
Conversation
…h/sway into config_block_comments
Gonna need help with adding assignees, adding labels, and requesting reviewers due to permissions. |
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.
Thanks for this!
swayfmt/tests/mod.rs
Outdated
|
||
configurable { | ||
// config test | ||
/// config test triple |
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.
Thanks for adding tests! It would be good to test inline comments as well, e.g. /* inline test */
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.
Ah, good catch! That style of comment is coming out a little strange after testing it just now. Weird behaviors with whitespace. Gonna dig into this a little more 👍
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.
After digging into the behaviors a little more, I discovered that this strange whitespace formatting behavior exists across other various items. I have put detailed it in this issue: #5298. Seems like what I found may be outside of the scope of this issue.
I added the /* inline test */
comment and it still exists after the formatting. However, the whitespace around it is a different thing. I had to form the testcase around this for it to pass.
Let me know what you think!
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.
Thanks for opening the issue. I agree that it's out of scope for this issue, but it would be great to have that test case added as part of #5298
thanks for full info |
swayfmt/tests/mod.rs
Outdated
// double slash comment | ||
/// triple slash comment | ||
SIGNER: EvmAddress = EvmAddress { | ||
value: ZERO_B256, |
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.
it would be also useful to test adding comments inside of structs and enums in configurable blocks, and at the end of the line.
value: ZERO_B256, | |
// comment here | |
// multiline! | |
value: ZERO_B256, // end of line too |
4b9256d
to
3e1562d
Compare
Going to be shelving this PR for the time being. Currently, there are weird behaviors with how end of line comments are handled inside of structs and inside of configurable blocks. My tests tell me it has something to do with the index that a comment is placed in and how newlines are handled regarding that. For example, manually shifting the insertion of formatted comments by -1 fixes the extra newline character in some cases. The issue is that it is not clear why or when these cases differ. Will be moving on to other issues as to not spend too much time here. Further explanation on the behavior and findings can be given if desired. |
Description
Closes #4966
Added functionality to retain comments in configurable blocks.
Checklist
Breaking*
orNew Feature
labels where relevant.