bacongravy / macinbox

Puts macOS in a Vagrant box
Other
662 stars 45 forks source link

Linting against, and introduction of, Rubocop Style Guide #35

Open ChefAustin opened 5 years ago

ChefAustin commented 5 years ago

Hi @bacongravy! Big fan of the project, so thanks for that.

I was curious if you would entertain a PR which would fairly drastically change the Ruby style written into the code to abide by Rubocop's 2.6.0 standards? There are ~1100 "violations" in its current state. I have already started working on a style update branch on my fork (down to ~600 "violations"), and would be happy to PR it over here if deemed to be amenable.

Cheers.

bacongravy commented 5 years ago

Hi! I’ll take a look if you want to point me at the branch. What advantage does adopting the rubocop 2.6.0 style bring this project? Does it make the code easier to understand?

ChefAustin commented 5 years ago

To give you an idea of some of the new style restrictions/changes that would be imposed upon the project by such a PR:

I think such a PR is a somewhat contentious topic, as style is arguably a personal thing. But in this case, the .rubocop.yml file could be amended to reflect the style, rules you would like to see abided by.

As far as what the project would gain from introduction of Rubocop:

  1. Clearly delineated guide on how code should appear making code review of contributions less likely to be off base.
  2. Rubocop is heavily used in the Ruby community, to the point where most IDE’s have plugins available for reporting linking errors on save.
  3. Not only would this help in terms of style, but also in terms of speed. I’ve found a handful of methods with what Rubocop called a “complex Assignment Branch Condition”. Usually this can help indicate functions which could/should be broken out into multiple methods.

Here is my forked branch: https://github.com/bacongravy/macinbox/compare/master...ChefAustin:chefaustin/lint-against-ruby-2.6.0

ChefAustin commented 5 years ago

Additionally, here is a gist containing the output of an initial pass of Rubocop over macinbox's master branch: https://gist.github.com/ChefAustin/088e684d42af05de436f79145716c028

bacongravy commented 5 years ago

Thanks. I don't have an objection to adding rubocop support, but it also doesn't seem very critical at this point. This project doesn't get many code contributions, so its not a big burden to bring contributions in line with my personal style.

I did review your forked branch and, in my opinion, I think many of those changes make the code less readable. My take on linting tools is that they are useful for shared code bases but require compromises to personal style. As an opinionated sole author of this tool, I don't really see the value in making those compromises when I'm not actually collaborating with anyone else. If I got a deluge of inconsistently-styled PRs, I'd happily reevaluate this stance.

You've put a fair amount of effort into this, so I assume it has value to you. The three things you mention seem to be benefits for project maintainers; in what way does adding rubocop to this project make this project better for you?

bacongravy commented 5 years ago

Some of the changes are obviously good or neutral; I don't object to using single-quoted strings when double-quoted aren't needed, or to swapping in ruby idioms like foo: 1 instead of :foo => 1, or using File.exist? instead of File.exists?.

ChefAustin commented 5 years ago

100% agree that this is not critical, and that by implementing such linting tasks might introduce some compromises to your personal style. That being said, I do think there is a middle ground that can be discovered (i.e. some of the acceptable changes you mentioned like single v. double quotes, key-value pair assignment within hashes, etc.).

This all being said, I'll incrementally PR some changes your way and I'll do my best to ensure that each PR is only making one specific, idiomatic change against the entire code base (for example: "PR#xx - Swap double for single quotes") as opposed to inundating you with a PR that makes many, radical changes to the project.

Thanks for your responsiveness and openness to my suggestion; it is much appreciated.