-
Notifications
You must be signed in to change notification settings - Fork 73
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
Remove blank lines after opening and before closing braces #1195
Remove blank lines after opening and before closing braces #1195
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Thanks. Out of experience (and because I produced bugs before), I know that removing line breaks is problematic when there are comments, also see https://styler.r-lib.org/articles/customizing_styler.html#showcasing-the-development-of-a-styling-rule. Hence: Can you modify some tests to cover situations with comments (and adapt the source code if necessary)? Also, the other thing we have to keep in mind is cache invalidation and transformer dropping. Here it seems not that they play a big role. Maybe we can also create developer documentation for that in |
I have added some tests with comments. PTAL. As for updating contributing guidelines, I think that can be handled in a separate PR, although it's not completely clear to me what needs to be added, but I can give it a go. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
see comment in R/rules-spaces.R
.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Looks good in general, I don't know if we need to boost the testing suite so much though (you are adding 500 lines of tests here)... One concern is that testing time increases (on CRAN, but also during the local development cycle) and also it becomes harder to navigate a big test suite for developers. Can you cut down on a test or two? |
Also, can you explain the rationale for making two different functions? I initially was scared it would be a performance problem, but seems not... In general I think we prefer separation of concern (as you did with the implementation of separate functions), but on the other hand having an abstract rule (line breaks around braces) should also correspond to one transformer, so I am not sure we should split it up here. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I see that they are different, but different is relative, and if you look at the code base, you can see that there are rules like call(
many = args
)
# vs
call(many,
args = 2
) By setting In other situations, e.g. for These things make me wonder how extra blank lines should be removed. Should there be only one fallback rule (which would be even greater in scope than those two that we have now) to set blank lines in general to at most one consecutive (exceptions to be defined) and other rules should maybe only deal with zero vs one blank line distinction? In case you did not notice, the order of transformers can also play a role. You put yours last. To me, we could also put this rule first (hopefully tests would all pass). I do appreciate this discussion with you, as I think it makes me think and explain the status quo (which is by no means perfect) and hopefully, this ultimately leeds to better code quality. |
@lorenzwalthert Thanks for the detailed explanation! Yes, makes sense now, and so I have switched to use only one transformer and I have also put it at the start, and all tests still pass. So this should be ready for another round of review. Blocked by #1208 |
@@ -58,12 +58,11 @@ function() | |||
1 ~ more() # comment | |||
``` | |||
|
|||
- More than one line break is tolerated before closing curly brace and line breaks between curly and round braces are not removed. | |||
- Line breaks between curly and round braces are not removed. |
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.
I don't think we should be tolerating such stray empty lines in any mode, strict or non-strict.
Such line breaks are rarely intentional, and are often the result of code being removed via "search and remove".
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This reverts commit 671f3f4.
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1195 +/- ##
==========================================
+ Coverage 90.76% 92.22% +1.46%
==========================================
Files 46 46
Lines 2652 2675 +23
==========================================
+ Hits 2407 2467 +60
+ Misses 245 208 -37 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if f9d4ab4 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
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.
Great, thanks for incorporating the requested changes.
Closes #1032
This is only one example. Please check newly added tests to get a feel for this change:
Created on 2024-05-01 with reprex v2.1.0