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: fix formatter adding -- to multiple use statements #773

Merged
merged 13 commits into from
Feb 13, 2022

Conversation

iqdecay
Copy link
Contributor

@iqdecay iqdecay commented Feb 11, 2022

This PR fixes #684

@iqdecay iqdecay self-assigned this Feb 11, 2022
@iqdecay iqdecay added bug Something isn't working formatter labels Feb 11, 2022
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Could you add a test that shows the behavior is fixed?

sway-fmt/src/fmt.rs Outdated Show resolved Hide resolved
@adlerjohn adlerjohn requested a review from digorithm February 11, 2022 14:38
@iqdecay
Copy link
Contributor Author

iqdecay commented Feb 11, 2022

Inadvertently stashed my test… For the rename, it's because of a weird formatter bug, I'm going to explicit it

Copy link
Contributor

@leviathanbeak leviathanbeak left a comment

Choose a reason for hiding this comment

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

This should be solved where the problem is (in handling of the UseStatement),
this current approach could even cause a bug if there are two lines that are the same, it would filter them as well.

Plus not sure why renaming that struct in a test is needed or connected to any of this?

Suggested solution:
in traverse_ast_node() where UseStatement is handled
if next_line of code is a UseStatement,
and if there already exists in changes the last_item.start that equals to next_line.start,
then they are the same UseStatement - so no need to handle it again, just ignore it.

* the renaming of the example struct `Lighthouse` and `Dart` is to have
  two different blocks of code. Otherwise the two code blocks  are
  exactly identical (incl. comment) and the formatter adds an extra
  newline.
@iqdecay
Copy link
Contributor Author

iqdecay commented Feb 11, 2022

Ok, implementing the suggested solution. It turns out that the "struct" renaming was indeed linked to 2 blocks of code being identical and a newline appearing because of it.

@leviathanbeak
Copy link
Contributor

It turns out that the "struct" renaming was indeed linked to 2 blocks of code being identical and a newline appearing because of it.

What were the odds lol

sezna
sezna previously approved these changes Feb 11, 2022
Copy link
Contributor

@sezna sezna left a comment

Choose a reason for hiding this comment

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

LGTM but left a question

sway-fmt/src/fmt.rs Outdated Show resolved Hide resolved
sway-fmt/src/fmt.rs Outdated Show resolved Hide resolved
sway-fmt/src/fmt.rs Outdated Show resolved Hide resolved
leviathanbeak
leviathanbeak previously approved these changes Feb 12, 2022
Copy link
Contributor

@leviathanbeak leviathanbeak left a comment

Choose a reason for hiding this comment

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

👍

adlerjohn
adlerjohn previously approved these changes Feb 12, 2022
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

digorithm
digorithm previously approved these changes Feb 12, 2022
sway-fmt/src/fmt.rs Show resolved Hide resolved
sway-fmt/src/fmt.rs Show resolved Hide resolved
@iqdecay
Copy link
Contributor Author

iqdecay commented Feb 13, 2022

I updated the "Temporary workaround" section about formatting, because, as shown by fe5016c, we actually do format non-compiling code.

Copy link
Contributor

@mohammadfawaz mohammadfawaz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding more test cases.

@iqdecay iqdecay merged commit 1631570 into master Feb 13, 2022
@iqdecay iqdecay deleted the vnepveu/fix-sway-formatter-imports branch February 13, 2022 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Formatter adding -- with multi-import statements
6 participants