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

Fixed SwayFmt removing comments in configurable blocks #5297

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

Conversation

brandonsurh
Copy link
Contributor

Description

Closes #4966

Added functionality to retain comments in configurable blocks.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@brandonsurh brandonsurh changed the title SwayFmt no longer removes comments in configuration block Fixed swayFmt no longer removes comments in configuration block Nov 21, 2023
@brandonsurh brandonsurh changed the title Fixed swayFmt no longer removes comments in configuration block Fixed SwayFmt removing comments in configurable blocks Nov 21, 2023
@brandonsurh
Copy link
Contributor Author

Gonna need help with adding assignees, adding labels, and requesting reviewers due to permissions.

@sdankel sdankel requested a review from a team November 22, 2023 00:55
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Thanks for this!


configurable {
// config test
/// config test triple
Copy link
Member

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 */

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

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!

Copy link
Member

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

@alipostaci2001
Copy link

thanks for full info

@brandonsurh brandonsurh marked this pull request as ready for review November 27, 2023 00:15
@kayagokalp kayagokalp added enhancement New feature or request formatter labels Nov 27, 2023
@kayagokalp kayagokalp requested a review from a team November 27, 2023 16:52
// double slash comment
/// triple slash comment
SIGNER: EvmAddress = EvmAddress {
value: ZERO_B256,
Copy link
Member

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.

Suggested change
value: ZERO_B256,
// comment here
// multiline!
value: ZERO_B256, // end of line too

@brandonsurh brandonsurh marked this pull request as draft November 28, 2023 17:48
@CLAassistant
Copy link

CLAassistant commented Dec 19, 2023

CLA assistant check
All committers have signed the CLA.

@brandonsurh
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SwayFmt removes comment in configuration block
5 participants