Add ForceEqualSignAlignment to .rubocop.yml #1190
Conversation
|
@ashmaroli you raised this originally, so will await feedback |
There was a problem hiding this comment.
Now that I see the extent of the damage to readability this can cause, I'm not so inclined on having this merged in the current state.
IMHO, the better alternative would be to not enforcing EqualSignAlignment globally but rather have the best of both worlds by aligning where it improves readability and forgoing the alignment if it hampers readability..
| @@ -892,7 +892,7 @@ Lint/FormatParameterMismatch: | |||
| Enabled: true | |||
|
|
|||
| Lint/HandleExceptions: | |||
| Enabled: true | |||
| AllowComments: true | |||
There was a problem hiding this comment.
I don't think it's a good idea to edit this file since it is auto-generated using upstream data.
If this change is absolutely necessary for this PR to pass, the better alternative would be to redefine the setting in the local .rubocop.yml
There was a problem hiding this comment.
This one comes down from a change to the Shopify style guide. I've been pulling this out of recent PR's
There was a problem hiding this comment.
I see.. Then you (@shopmike) should push this change to master directly then. It may not be ideal but a lot better than pulling the change out from multiple PRs..
Shopify could also consider shipping the Official Style Guide as a Ruby gem so that its updates do not explicitly show up in downstream repos.
There was a problem hiding this comment.
Yeah, the plan is to push this through as its own PR, just hasn't been a priority.
The config rarely changes and when it does I don't mind the fact it forces everyone to follow it as generally it would only be done when enough people have agreed that it applies to all projects.
lib/liquid/block_body.rb
Outdated
| unless (tag = registered_tags[tag_name]) | ||
| # end parsing if we reach an unknown tag and let the caller decide | ||
| # determine how to proceed | ||
| return yield tag_name, markup | ||
| end | ||
| new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) | ||
| @blank &&= new_tag.blank? | ||
| new_tag = tag.parse(tag_name, markup, tokenizer, parse_context) |
There was a problem hiding this comment.
Does RuboCop force this realignment if you were to revert manually and then autocorrect after?
The reason I ask is the benefits of operator-alignment is lost when there is a block of code separating two realigned lines..
There was a problem hiding this comment.
I think the only thing that breaks the behaviour is an empty line from when I was playing around with this. In cases where the formatting isn't ideal, you need to space it.
lib/liquid/block_body.rb
Outdated
| @@ -92,7 +92,7 @@ def parse(tokenizer, parse_context, &block) | |||
| end | |||
| parse_context.trim_whitespace = false | |||
| @nodelist << token | |||
| @blank &&= !!(token =~ WhitespaceOrNothing) | |||
| @blank &&= !!(token =~ WhitespaceOrNothing) | |||
There was a problem hiding this comment.
This is the side-effect of using automated corrections.. 😞 :
As a human, you can tell from a distance that this change significantly hampers readability instead of improving it..
(View the entire file for proper judgement).
There was a problem hiding this comment.
This is a good way to be able to find where needing to space should be done. Searching for 15+ spaces provides most of the weird auto-corrections in which case I would space
lib/liquid/context.rb
Outdated
| @@ -85,9 +85,9 @@ def pop_interrupt | |||
| end | |||
|
|
|||
| def handle_error(e, line_number = nil) | |||
| e = internal_error unless e.is_a?(Liquid::Error) | |||
| e = internal_error unless e.is_a?(Liquid::Error) | |||
There was a problem hiding this comment.
It would be better if you could replace single letter variables with something longer yet apt to mitigate this excess whitespace..
If its not too much trouble, I recommend that you follow this suggestion for all similar scenarios
There was a problem hiding this comment.
I think the only issue with most of these examples is that by making the variable longer, the next line is usually the same variable but accessing long method names or nested methods.
There was a problem hiding this comment.
should we keep this cop and try to address these cases individually by small refactoring to improve readability or should we postpone the enforcement of this cop?
As now we have a reference on all equal alignments in the code base, should we revert the ForceEqualSignAlignment cop and leave all the changes that make sense(reverting cases that don't bring much benefit)? |
@alebruck I would prefer that if you don't mind the extra effort. Either ways, I'm thankful that you attempted to resolve this issue. |
|
@ashmaroli Could you please check this one again? No new cops but the alignment improvements required for the cop are already here. I would leave any refactoring related to var name in a different PR as it looks like we could use more refactoring than just renaming. |
|
@alebruck I had sent you a PR on your fork: alebruck#1 |
|
@ashmaroli oh, somehow I got no notification from that, sorry about that 😅 |
|
@shopmike My concerns have been addressed. |
test/unit/tokenizer_unit_test.rb
Outdated
| tokens = [] | ||
| while (t = tokenizer.shift) | ||
| tokens = [] | ||
| while (t = tokenizer.shift) |
There was a problem hiding this comment.
This is about the only one that doesn't seem to make sense.
There was a problem hiding this comment.
Ah! I missed that! 😛
It should be something like:
tokenizer = Liquid::Tokenizer.new(source)
tokens = []
while (t = tokenizer.shift)I can't push to this branch directly,.. so its your call..
|
Awesome thanks for resolving this outstanding issue @ashmaroli @alebruck . Also congrats on your first contribution to Liquid @alebruck 🎉 |
Fixes #1169 by adding cop
ForceEqualSignAlignmentand updating all reported issues.For now, I had to disable
SpaceAroundOperatorsas this cop seems to have an issue that causes an infinite loop(not possible to achieve a green state). I reported it on rubocop rubocop/rubocop#7422 and I can enable it again once this issue is resolved.