-
Notifications
You must be signed in to change notification settings - Fork 85
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
Enforce RuboCop and correct existing syntax #117
Conversation
This enables RuboCop by default and fixes the existing offences
sodium_constant :BOXZEROBYTES | ||
sodium_constant :BEFORENMBYTES | ||
sodium_constant :PUBLICKEYBYTES | ||
sodium_constant :SECRETKEYBYTES, :PRIVATEKEYBYTES |
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.
Stuff like this with rubocop really bugs me. I think version where they're all aligned to the same vertical is a lot more readable.
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.
RuboCop is configurable. We can shut this particular one off. And FWIW I agree. This patch brings us close to in-line with the RuboCop defaults, so... let the bikeshedding begin! 😉 Then we can shut off the cops we dislike
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.
@namelessjon I disabled the SingleSpaceBeforeFirstArg cop and put these back to what they were. Any other objections?
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.
No, I think the rest is all good!
A lot of it makes sense, but some of the stylist choices bug me, and I don't think they help with the readability. On the other hand, being able to push toward shorter methods is good. |
If you have no other objections, merge it? 😉 |
Enforce RuboCop and correct existing syntax
Done |
This enables RuboCop by default and fixes the existing offences