(PUP-3036) Rubocop Style/AndOr autocorrect pass #2944
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CLA signed by all contributors. |
vrthra
changed the title
vrthra
changed the title
This commit replaces `and` and `or` keywords in favor of `&&` and `||` where it can be done automatically (In places where the semantic meaning of the expression does not change on replacing the `and` keyword with the operator `||`). Since we are using `rubocop -a` to make the changes, we are assured of the correctness of translation. These changes have also been individually verified. We have been able to get rubocop to make further changes than previously by identifying equivalent patterns identified as different in parsing (See rubocop issue #1255)
This commit replaces `not` in favor of `!` where can be done automatically (In places where the semantic meaning of the expression does not change on replacing the `not` keyword with the operator `!`). Since we are using `rubocop -a` to make the changes, we are assured of the correctness of translation. These changes have also been individually verified. This change is especially useful because after replacing such instances, the AndOr cop can autocorrect even more instances of violations This also enables the Style/Not cop
This commit ensures that no space is used between the application of the operator `!` and its operand. That is, it replaces the instances of `! mymethod` with `!mymethod` It also enables the Style/SpaceAfterNot cop
This commit replaces `and` and `or` keywords in favor of `&&` and `||` where it can be done automatically (In places where the semantic meaning of the expression does not change on replacing the `and` keyword with the operator `||`). Since we are using `rubocop -a` to make the changes, we are assured of the correctness of translation. These changes have also been individually verified. This commit takes advantage of the previous changes replacing `not` keyword to autocorrect even more instances of violations.
This commit replaces `AndOr` in favor of `and` and `or` where can be done automatically (In places where the semantic meaning of the expression does not change on replacing the `and` keyword with the operator `or`). Since we are using `rubocop -a` to make the changes, we are assured of the correctness of translation. These changes have also been individually verified. The difference in this commit is that, we specifically search for instances where the semantics of `and` or `or` is unchanged from `&&` and `||` when parenthesis are introduced for method calls. That is, we look for instances of the form `mymethod a,b or mymethod c` and convert them to `mymethod( a,b ) or mymethod( c)` before applying the transformation.
This commit replaces `AndOr` in favor of `and` and `or` where can be done automatically (In places where the semantic meaning of the expression does not change on replacing the `and` keyword with the operator `or` after some preprocessing). Since we are using `rubocop -a` to make the changes, we are assured of the correctness of translation. These changes have also been individually verified. The difference in this commit is that, we specifically search for instances where the semantics of `and` or `or` is unchanged from `&&` and `||` when parenthesis are introduced for expressions with assignments. That is, we look for instances of the form `if a = b or c` and convert them to `if ( a = b ) or c` before applying the transformation. These changes have been restricted to conditionals in the interests of readability of resulting expressions.
This commit updates the previous automatic pass where the method calls of the format `method a,b or c` was changed to `method( a,b ) || c` to remove the leading and trailing spaces -- to `method(a,b) || c`.
|
We're about to remove the puppet-4 branch as part of the start of work on Puppet 4 proper, so this should be targeted as master. If it gets unexpectedly closed then it means that the branch deletion closed this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces
andorandnotkeywords in favor of&&||and!where itcan be done automatically (In places where the semantic meaning of the
expression does not change on replacing the
andkeyword with the operator||).This PR also makes the spacing of
!operator consistent, changing! mymethodto!mymethodSince we are using
rubocop -ato make the changes, we are assured of thecorrectness of translation. These changes have also been individually verified.
Rubocop previously relied on the parser results to check the equivalency of new
and old constructs after autocorrect. However, for our purposes, it is rather
restrictive since parser can produce different AST depending on the context,
rubocop misses quite a large number of autocorrect possibilities. My fix to
rubocop involved supplying that context so that it checks for the equivalence
better.
While incorporating parenthesis for method calls, I noticed that puppet codebase has not
standardized on a method call spacing. that is, I find instances of
method( ... ),method(...)and evenmethod( ...)in different places. For the purposes of thisPR, I have used
method(...)(simply wrapping with parenthesis). Rubocop recommendsmethod(...). After the puppet-dev discussion, the consensus seems to be the same, (but with spacing between arguments -- this will be taken up in a future PR)The remaining violations are in these directories.