-
Notifications
You must be signed in to change notification settings - Fork 170
Fix rubocop violations #320
Fix rubocop violations #320
Conversation
charlesjohnson
commented
Feb 16, 2015
- Add a .rubocop.yml to the generator cookbook_files
- Updated app generator to write out rubocop.yml
- Updated cookbook generator to write out rubocop.yml
- Added .rubocop.yml to chefignore
- Fixed rubocop violation in Berksfile
- Fixed rubocop violations in metadata.rb
- Fixed rubocop violations in serverspec default_spec.rb
- Fixed rubocop violations in chefspec default_spec.rb
- Add a .rubocop.yml to the generator cookbook_files - Updated app generator to write out rubocop.yml - Updated cookbook generator to write out rubocop.yml - Added .rubocop.yml to chefignore - Fixed rubocop violation in Berksfile - Fixed rubocop violations in metadata.rb - Fixed rubocop violations in serverspec default_spec.rb - Fixed rubocop violations in chefspec default_spec.rb
Some notes:
|
StringLiterals should also be turned off, as per rubocop/rubocop#520 even bbatsov recognizes that the community is split on this one, and I positively can't stand it. Generally rubocop is going to be quite a bit of a bikeshed which is why we left it off initially I think |
See #37 for other discussion. Between the cops where Rubocop doesn't do the right thing (e.g. 'rescue Exception` followed by immediately re-raising the exception is perfectly valid), tries to engineer a brand new community standard where there isn't one (e.g. replacing 'raise' with 'fatal' because someone bbatsov is impressed with suggested it at one point), tries to engineer a standard that i'd argue goes against the community (e.g. suppressing trailing commas on multiline hashes and arrays), and just picks one side by default in a bikeshed war (StringLiterals), I don't think there's any way to arrive a decent consensus. |
THE QUICKENING BEGINS. See, I like StringLiterals. Clearly we must fight with knives. Persona bias right up front: I see lots of value in Rubocop, it definitely helped me level up a lot of my code as a sysadmin-turned-Chef. In the absence of a defined style guide / standard for Ruby in general and Chef in particular, I agree that de facto solutions such as Rubocop can be aggravatingly fluid, misguided, and flat-out wrong. But also, right now CHEF has a small army of customer-facing engineers/consultants engaging with the community and preaching Rubocop as good practice for Chef code. We also ship the tool as part of the ChefDK. By having no .rubocop.yml in the generator, we're saying that Every. Single. Cop. is valid for Chef cookbooks, which I think we CAN agree is pretty much madness. If we're shipping Rubocop then some set of almost-sane defaults seems a reasonable thing to ship with it. If people don't like those defaults, they can configure their own. That's why .rubocop.yml exists, along with custom generator support. Also, what we ship here can evolve over time. Yes, there's probably no way to reach a happy consensus on this, but I think we can find a everyone-grumbles-about-it consensus. However, if we can't come to consensus, then perhaps another PR ripping out Rubocop completely is in order? What I don't want to see is that CHEF continues shipping it and preaching it, while also leaving it with insane defaults. |
Yeah, but are we preaching it? @sethvargo invented preaching about it in the chef community then backed way off after he felt they jumped the shark quite a bit. I also think its got some utility, but it needs to be put onto a fairly tight leash. FWIW my standard .rubocop.yml looks like this:
SpaceInsideBrackets and SpaceInsideParens are also things that drive me nuts. I like to leave a little whitespace to let expressions visually 'breathe' a bit to add some clarity. SingleSpaceBeforeFirstArg needs to be off for providers/ and recipes/ as well, or else you can't justify you arguments together, which I definitely like to do: file do
owner "root"
group "root"
mode "0644"
end |
Having literally gone 360° on this, I agree with @lamont-granquist on this one. Let me elaborate... It really boils down to this - at the end of the day Chef != Ruby. Sure, Chef is written in Ruby, but recipes are a highly specialized DSL that does not fit in the guise of standard Ruby programming. At the end of the day, style is a team decision. As a single individual writing cookbooks, style matters not. Once I share the responsibility of that cookbook maintenance with my team, however, a style begins to emerge. This is highly variant based on the individuals involved, and it is also an evolving process - you don't just BOOM, style. The argument that open source cookbooks should follow a common style is also lost on me. Maintainers should totally decide the style that works best for their repos, but that's not Chef Software's decision to enforce tbh. If Sean wants to use Rubocop, that's great, but that doesn't mean every user should, or that every user should have the same set of rules either. Each project is unique and has its own style which is up to the maintainer(s) to enforce with whatever tools they please. From a purely economic perspective, the cost-benefit-analysis does not pay off. "Errors" (if you can call them that) are confusing to new users. You are only increasing the already huge learning curve for Chef. You must learn x, y, z, and now w before you can write Introducing Rubocop to the Chef community is one of my biggest regrets. If I could go back and change it, I would. But I can't. So here we are. I would heavily discourage this addition. |
So, TL;DR the solution to the problem is not to add a |
Yep, I'd prefer to see PRs that refactored code into tighter objects with better specs, which is 100x the payoff towards a better codebase than style fixes. |
If consensus is that the best way to make ChefDK awesome is to rip Rubocop out completely, I'm down. As I said, what I don't want to see is that CHEF continues shipping Rubocop and preaching it, while also leaving it with an insane set defaults. |
Thing is that right now we just ship it, we don't wire it up in any way as far as I know, so its just there if you want to use it. If you start using it, I think you inherently start needing to buy into maintaining your own .rubocop.yml file and that needs to go into your own custom generators. |
I still believe rubocop is a helpful tool to allow teams to maintain some style consistency. But I also agree that the style should reflect our Chef environment more so then a pure Ruby one. I think it would be great to have ChefDK continue to include rubocop AND include an example rules file that focuses on the basic style most cookbooks already have (like the one above). |
This is an illuminating thread, with lots of good discussion. I feel pretty strongly that "do nothing" is a bad option. "Here is a tool BUT DON'T YOU DARE USE IT" is a lousy thing to say to a user. "Here is a tool but the output is bonkers unless you write your own customized config dotfile in yaml, and locating the docs for that config file and determining its location in your project is left as an exercise to the user" is an even more lousy thing to say. ChefDK's purpose is to give the user tools to be awesome at writing Chef code. By bundling Rubocop we're tacitly blessing it as a tool for that purpose. By bundling it without a .rubocop.yml in the generator, we're tacitly blessing Rubocop with AllCops enabled on every file. Which I think we all agree makes it useless out of the box. So if we leave it in, then I think we have to have the bikeshed, and at least dictate some starting point for a community standard. And if we rip it out, then we don't have the bikeshed, but also we don't have the tool, and maybe that's okay? But I'd like to hear from more people on that one. As to how widely it's used: CHEF currently teaches Rubocop as part of the Intermediate Chef training, so if we make this change then that will need revision as well. |
Mark me down as a +1 for shipping rubocop + a curated .rubocop.yml. The whole point of the DK is to put people in The Right Path (tm). Having a tool that can do automated lint correction (rubocop -a) is a [ "having" %w( syntactically valid ways) %w{to do a thing} is actually hostile to new The default rubocop rules are annoying, but there is a LOT of good stuff in Shipping a Chef Software Recommended starting rubocop.yml, with commented -s On Tue, Feb 17, 2015 at 1:32 PM, Charles Johnson notifications@github.com
|
I just don't see how shipping rubocop without any wiring at all is tacitly blessing all cops. Also WRT this statement: "Here is a tool but the output is bonkers unless you write your own customized config dotfile in yaml, and locating the docs for that config file and determining its location in your project is left as an exercise to the user" That is exactly what you've got now when you gem install rubocop. That feels like a statement of fact about the way the world is. They didn't write rubocop to be minimally invasive and generally correct out of the box and instead wrote it with a ton of opinions so to make it usable you have to turn them off. Yet some people may like all the cops being turned on as well (I know there's arguments on twitter suggesting exactly that). |
And when it comes to |
I fail to see how pasting around a 50+ line YAML file to every cookbook (and then changing that YAML file each time rubocop decides to include another rule) is a productive use of anyone's time. The simple fact is that it really doesn't matter. We should be encouraging users to pick a style of their choosing. You have to write recipes this way seems to go against the original founding principles of Chef. |
CHEF promises paying customers contractual support for all things in the ChefDK package. Thus all of the things that are included in the ChefDK package are blessed by sheer virtue of that inclusion. In addition, nothing has been explicitly wired together in the ChefDK, so that would imply that none of the package contents are blessed at all. Providing a standard is not the same as _telling people they _have* to do things a certain way.* CHEF advocated for single repository development for years before Berks came along and blew that up. Standards provide guidance, and a point to argue with if that guidance is not positive. Standards can evolve. Plus there's already a standard for Rubocop defined in the ChefDK, and that standard is "use Rubocop with AllCops," which I think is confusing, harmful, and should not remain. Thus this thread. (I'm intentionally ignoring syntax/style arguments for a moment. I'd rather focus on "should we include Rubocop at all" as the question.) |
No. My vote is a very hard "no". |
This less about the "philosophy of freedom" than it is about "the reality It really boils down to two questions: Do we lose anything by shipping a .rubocop.yml in the default generator? Losses:
Wins:
My view on this is colored by spending large amounts of time dealing with -s On Tue, Feb 17, 2015 at 2:35 PM, Seth Vargo notifications@github.com
|
Personally, I'd be quite receptive to an outcome where we could say "some teams find enforcement of style rules to be a useful process to improve code readability within/across working groups. If you'd like to opt-in, Here's a set of sane defaults you can easily tailor to your needs. And we have a sane way of upgrading this component without causing a bunch of test failures or maintenance hassles." I don't think we can do this with rubocop because:
|
I still fail to understand how shipping rubocop installed but without any default wiring becomes a 'standard'. However, given that, then I'm in agreement with dan and seth that it simply needs to get dropped and users need to chef gem install or bundle install the exact version they desire. |
+1 on dropping if we're not going to ship sane defaults -s On Tue, Feb 17, 2015 at 4:16 PM, Lamont Granquist notifications@github.com
|
Is there another ruby-level linter that allows for rule whitelisting vs On Tue, Feb 17, 2015 at 5:02 PM, Sean OMeara someara@chef.io wrote:
|
+1 here for keeping Rubocop and providing sane defaults as well, in case that's not clear by the PR itself. The counter-arguments are compelling but it still seems a least-worst option. |
+1 for keeping rubocop (convenient to have tools available) ...as for providing defaults, that's nice but not necessary. |
+1 from me on sane default. |
I know everyone wants a sane default, I do, too. But again, I don't have the slightest idea how that's possible with rubocop without significant downside:
|
I'm with you Dan. For example, some time ago the name of the 'todo' file changed. The generated .rubocop.yml doesn't need to have all of the rules activated, they can (and perhaps should) all be there commented out. Just having the scaffolding is a great step forward with much less overhead. # inherit_from: .rubocop_todo.yml
# LineLength:
# Max: 200
# etc... |
Just ran across someone wrapping rubocop into their own gem with their own default style: |
Out in the field, working with Chef newbies (who are also Ruby newbies), I've been finding a lot of value with Rubocop. As @charlesjohnson mentioned, it has personally helped ME level up my Ruby as a non-coder, which a lot of our customers are. When I teach a POC to a group, I always use Rubocop (after giving them my own .rubocop.yml, which I stole from @charlesjohnson who I think stole it from @someara). I can see how this seems redundant or inappropriate for deep Chefs, who already are good at this stuff, or for a team that is already working. But for the "we are just getting started", everything we can do to make it easier and cleaner helps them move faster. So I'm +1 for still shipping Rubocopy with the DK, but with at least a base .rubocop.yml that removes some of the more useless cops. |
My suggestion: Use |
RuboCop helps people learn Ruby, and makes folks who are new to the ecosystem happier writing code. Happy coders make happy code. Iterating towards the most readable standard should be part of the journey. Everyone out there in the community has varying degrees of experience, and varying levels of success at achieving their goals from month to month. Really good teams are rare, and they don't really last that long. There's a reason why Times New Roman looks the way it does, and Helvetica looks the way it does. Readable code is more readable, and there are multiple contexts for readability. I advocate starting with more firm rules, and relaxing or altering them over time as needed. Starting with a style guide is certainly better than none and will result in more readable code, which means fewer errors over time. |
The |
A little bike shedding can be a good thing. %w(this works) -s On Mon, Mar 2, 2015 at 11:55 PM, Lamont Granquist notifications@github.com
|
It seems like rubocop can now run in an opt-in mode if you use configuration like this:
cf. https://github.com/bbatsov/rubocop/blob/master/README.md#enabled So I think we can now actually make progress on this and have a default config that won't break the universe on every rubocop upgrade. This solves my number one objection to rubocop. I would be happy with a config that makes all the rules opt-in and does not enable the bike shed garbage rules. And for the love of all that is good, never enable the string literal thing. |
Turning off all cops by default doesn't seem ideal as it disables the lint cops which catch many code errors. The best long term solution is a shipping a "chefstyle" wrapper for RuboCop that works in a similar way to https://github.com/fnichol/finstyle. I believe @fnichol and @robbkidd began scheming this at the community summit hack day. It would be nice to have 2 sane sets of rules:
A final thought - we should listen to the words of wisdom from our community, customers and Chef's customer facing teams: most new Chef users find a lot of value in RuboCop (even with its warts). Project maintainers and more experienced users also benefit from allowing a tool to tell new contributors when their code is not up to par! Code style and consistency is important. |
My point is you disable everything, then enable a set of things that are actually good (personally, the lint rules are the only ones I actually like). It'll probably make the rubocop yaml pretty big, but the benefit is that when a new version of rubocop comes out, it doesn't enable a bunch of new rules that fail your previously passing builds. Thus we can actually use rubocop as part of a stable project that people can upgrade with confidence. The opt-out mode of configuration is not acceptable because new rules can fail your builds on upgrade, which is not acceptable for stable software. |
Yeah, as Lamont said we were discussing |
With the addition of opt-in configuration mode, I think we can get pretty close to optimal with rubocop. Things that I predict would still be less-than-perfect:
Despite that, at this point I think it's still a win to start by putting a rubocop config in the generators, so we can avoid issues like #611 |
Yeah the problems with the massive config file is what leads me to want to bundle that up in a gem and ship it as |
Can this get closed now that we have chefcop applied to both chef and ohai. A new PR applying chefcop here seems like it would make more sense. Ping Mr. Rubocop @lamont-granquist |
@tas50 So this ticket is mostly about making our generated cookbooks (and Berksfile and Policyfile.rb files) rubocop-valid. I had thought we settled on making two rubocop derivatives--one for our ruby applications (which is In either case we'd need to do at least a little more work, so this isn't done. But maybe we replace it with a new ticket or a feedback.chef.io thing? I don't have strong opinions on how we track it. |
So, chefstyle is still about our code, not user code, and that isn't going to change for awhile, its still not done. It would probably be better to fork off a cookstyle or just chefcop that was recommended rulesets for cookbooks. Here's the TODO list for chefstyle right now (these are offenses which chefstyle disables by default but we haven't made a decision on if or how to enable):
|
for future posterity, we went with cookstyle: https://github.com/chef/cookstyle it is for cookbook usage. the similar 'chefstyle' gem is for chef/chef and related ruby code. |