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

Formatter adds a space to start of block scope #2932

Closed
Braqzen opened this issue Oct 4, 2022 · 14 comments · Fixed by #5007
Closed

Formatter adds a space to start of block scope #2932

Braqzen opened this issue Oct 4, 2022 · 14 comments · Fixed by #5007
Assignees
Labels
bug Something isn't working formatter good first issue Good for newcomers

Comments

@Braqzen
Copy link
Contributor

Braqzen commented Oct 4, 2022

The following code, when formatted, will add a space after the foo declaration line but right before the { so that it becomes <space>{.

library scope;

fn shadowing_by_scope() {
    // ANCHOR: shadowing
    let foo = 5;
    {
        let foo = 42;
    }
    assert(foo == 5);
    // ANCHOR_END: shadowing
}

Post formatting:

library scope;

fn shadowing_by_scope() {
    // ANCHOR: shadowing
    let foo = 5;
     {
        let foo = 42;
    }
    assert(foo == 5);
    // ANCHOR_END: shadowing
}
@Braqzen Braqzen added bug Something isn't working formatter labels Oct 4, 2022
@eureka-cpu eureka-cpu added the good first issue Good for newcomers label Oct 9, 2022
@yash251
Copy link
Contributor

yash251 commented Dec 14, 2022

please assign me

@yash251
Copy link
Contributor

yash251 commented Dec 21, 2022

which file is this code in?

@Braqzen
Copy link
Contributor Author

Braqzen commented Dec 21, 2022

which file is this code in?

The code can be found here; however, if you take a look at this issue and that code you will notice that there is no space because it has been manually removed.

This is one example that you may use when trying to fix the issue in the sway formatter. Alternatively, the snippet has been posted in the issue as a minimal reproducible sample so you should be able to create a brand new forc project and paste the code from the issue in there. If you run the formatter, assuming it hasn't been fixed since this issue was posted, then the snippet should "break" into the post-formatting sample. If that is the case then some code inside the formatter needs to be fixed.

@AliJafriETH
Copy link
Contributor

Hey @Braqzen, is this issue still open?

@eightfilms
Copy link
Contributor

Hey @AliJafriETH, I can confirm that the issue still exists on the master version of the formatter. Would you like to have a shot at this?

@AliJafriETH
Copy link
Contributor

Hey @bingcicle, thank you so much 👍
Would love to work on the issue, might take some time as i'm currently having my uni exams :'(

@eightfilms
Copy link
Contributor

@yash251 - are you still working on the issue? If so, do let us know if you encountered any blockers.

@AliJafriETH since the issue was assigned to @yash251 initially, I'd like to first see if @yash251 would like to continue working on it first, so that we don't end up wasting your time doing duplicate work. :/

@eightfilms
Copy link
Contributor

@AliJafriETH I see now that you're also assigned to #3474, perhaps you might want to focus on that first?

@AliJafriETH
Copy link
Contributor

Hey @bingcicle, would work on #3474 first 👍
Thanks 👍

@AliJafriETH
Copy link
Contributor

Hey @bingcicle, just submitted the PR for #3474, is this issue still open?
Thanks 👍

@eightfilms
Copy link
Contributor

eightfilms commented Jan 30, 2023

@AliJafriETH first of all, thanks for your PRs!

With that said, I took a look at the PRs and it seemed like there are some misunderstandings about the requirements of the issue - please take a look at @eureka-cpu's comment here.

Once the comments are resolved and if @yash251 hasn't responded, we can assign you this issue as well.

Please feel free to ask questions!

@yash251
Copy link
Contributor

yash251 commented Jan 30, 2023

where can I find it's reference in the sway book?

https://fuellabs.github.io/sway/v0.33.1/book/basics/variables.html

@eightfilms
Copy link
Contributor

where can I find it's reference in the sway book?

https://fuellabs.github.io/sway/v0.33.1/book/basics/variables.html

Hmm, I'm assuming you misunderstood the issue - this issue isn't related to language implementation, rather its about formatting. You can take a look at the swayfmt CONTRIBUTING.md to get started :)

@eightfilms
Copy link
Contributor

eightfilms commented Feb 7, 2023

Hey @yash251, following up from your forum question here:

Firstly, please make sure you've read the CONTRIBUTING.md linked above before starting to work on this.

Basically, in the original example posted in this issue, the code should not be changed at all, since the formatting is correct.

However, the formatter is currently putting a space right before the 2nd opening curly brace, after the statement let foo = 5;.

The diff (red is what we don't want, green is what we want - you can see there's an extra space inserted before the curly brace).

library scope;

fn shadowing_by_scope() {
    // ANCHOR: shadowing
    let foo = 5;
-     {
+   {
        let foo = 42;
    }
    assert(foo == 5);
    // ANCHOR_END: shadowing
}

The entire formatter lives within swayfmt, so getting to know that directory should be enough. It does use sway-parse and sway-ast to parse and understand the AST, but that shouldn't be in scope for this issue. There is a Format trait implemented for tokens that the formatter is interested in formatting, and within their implementations you will find how they're formatted. The ItemFn impl should probably be what you want here.

To test out your changes, I'd recommend creating a test within tests/mod.rs, and running cargo test <TEST_NAME> to see if your changes are correct, instead of running the formatter directly.

Please feel free to ask follow-up questions here!

sdankel added a commit that referenced this issue Aug 23, 2023
## Description

Closes #2932

## Checklist

- [x] 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](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] I have requested a review from the relevant team or maintainers.
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 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants