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

Leading blank lines in a scope are not removed when a comment present #951

Closed
IndrajeetPatil opened this issue Jun 13, 2022 · 7 comments · Fixed by #952
Closed

Leading blank lines in a scope are not removed when a comment present #951

IndrajeetPatil opened this issue Jun 13, 2022 · 7 comments · Fixed by #952

Comments

@IndrajeetPatil
Copy link
Collaborator

without comment

styler::style_text(
  'test_that(
  desc = "bla",
  code = {

    expect_equal(1 + 1, 2)
  })'
)
#> test_that(
#>   desc = "bla",
#>   code = {
#>     expect_equal(1 + 1, 2)
#>   }
#> )

with comment

styler::style_text(
  'test_that(
  desc = "bla",
  code = {



    # comment
    expect_equal(1 + 1, 2)
  })'
)
#> test_that(
#>   desc = "bla",
#>   code = {
#> 
#> 
#> 
#>     # comment
#>     expect_equal(1 + 1, 2)
#>   }
#> )

Created on 2022-06-13 by the reprex package (v2.0.1)

@lorenzwalthert
Copy link
Collaborator

This is intended. SEE NEWS.md:

blank lines in function calls and headers are now removed, for the former only when there are no comments before or after the blank line (#629, #630, #635, #723).

@IndrajeetPatil
Copy link
Collaborator Author

But, if I am not mistaken, this is neither a function call nor a function header, but rather a code block, right?

In any case, do you not think this should be the expected behaviour?

This is quite likely to happen because developers add many skip_ conditions for a given test block, and search and replace them with empty lines later when they are no longer needed. So it will be nice if styler could clean up these unnecessary empty lines between the opening brace and the test code.

@lorenzwalthert
Copy link
Collaborator

Ok fine... Without adding more complexity, this will result in comments being moved to the next line though if they are on the same line as the opening brace. This is fine thouth I think, as { should be the last character on a line anywyas according to the style guide.

@lorenzwalthert
Copy link
Collaborator

@IndrajeetPatil thanks for the issue. I should just say though that I try to reduce my time spent on {styler} and that I don't know if I will fix issues with (for me) low priority in the future anymore. Maybe others like @krlmlr will jump in. If you feel like you want to contribute, I am happy to review pull request for issues like this. Just you know. Of course you can always file issues and unless they are not legit, I won't close them 😄

@lorenzwalthert
Copy link
Collaborator

This issue should be fixed iwth #952. You can inspect the diff to get a feel for how I did it.

@IndrajeetPatil
Copy link
Collaborator Author

Thanks for the quick fix, Lorenz!

If you feel like you want to contribute, I am happy to review pull request for issues like this.

I'd definitely like to contribute more, and would start doing so when I have a bit more time after July (especially #936 and #759).

@lorenzwalthert
Copy link
Collaborator

Great, I am happy to provide guidance in that. It's not that I want to stop working on {styler} tomorrow, but probably mid-term, e.g. start reducing gradually and onboard new contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants