-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
Update RuboCop to new gem version (0.23.0). Include New RuboCop file layout specified in [this](rubocop/rubocop#1050) issue. Update Rakefile to use new RuboCop class definition. From Rubocop to RuboCop. Add new .rubocop_todo.yml file following new RuboCop file structure. Signed-off-by: "Jake Champlin" <jake.champlin.27@gmail.com>
Apply RuboCop Auto Fixes for code style issues per new Gem Version (0.23.0) Signed-off-by: "Jake Champlin" <jake.champlin.27@gmail.com>
mode '0644' | ||
owner 'root' | ||
group 'root' | ||
mode '0644' |
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.
So I actually don't like this rule. I think, in Chef land, it's nice to align the parameters for readability.
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 agree.
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'm kind of torn on this. I like how it looks, but @sethvargo taught me to put trailing commas on my hashes so the commit doesn't show changes on lines where nothing really changed.
Doesn't this present the same problem? If I add a line where the first thing is longer than everything else there, I have to realign everything.
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.
@smith excellent point, but git is actually smart enough to ignore whitespace. So if you -w0 locally or add ?w=1
to the GitHub URL, you'll see the real diff. Trailing commas, on the other hand, are not whitespace chars so they will be used.
I, too, am torn: https://www.youtube.com/watch?v=sYAhku6gKQA
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 like how it looks before fix. Personally agree with @sethvargo on aligning parameters.
I can explicitly disable the SingleSpaceBeforeFirstArg, but that would change recipes/_postgres.rb
back a commit.
In a world where people do cookbook development using ChefDK, they wouldn't need to use bundler to install the gems for a cookbook, as ChefDK will have the best of breed tools, at versions that are known to work well together. Currentl versions of rubocop made backwards incompatible changes that break its config directives. While that isn't an unexpected event in a pre-1.0 tool, many cookbooks that use rubocop already have .rubocop.yml defined with the old naming of things. Unless something changed that I haven't seen in the last couple weeks, we've pinned rubocop to version 0.18 everywhere to avoid updating the configs everywhere. A change like the one proposed in this PR may lead to breakage and "gem update hell" for those who are using ChefDK, or otherwise pinning rubocop in their projects/environment. I know not everyone uses ChefDK for their "Chef Development Environment" - however that will be the best supported thing to do in the future, and many employees at Chef already use it. We encourage the community to adopt and use it, and hopefully we can avoid a world of mismatched Rubies, bundled gems, and workflows. |
@jtimberman, makes sense to me. I have no objections to closing this PR either should it be better for the ecosystem as a whole. |
Update RuboCop to new gem version (0.23.0).
Include New RuboCop file layout specified in this issue.
Update Rakefile to use new RuboCop class definition.
From Rubocop to RuboCop.
Add new .rubocop_todo.yml file following new RuboCop file structure.
Signed-off-by: "Jake Champlin" jake.champlin.27@gmail.com