Skip to content
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

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

Open
ChefAustin opened this issue Mar 31, 2019 · 6 comments
Open

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

ChefAustin opened this issue Mar 31, 2019 · 6 comments

Comments

@ChefAustin
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

ChefAustin commented Mar 31, 2019

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

  • Max line length would be 80 characters
  • Unless needed string interpolation is needed, single quotes are favored
  • Method, class blocks are restricted to a defined number of lines
  • Among many other things

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:
master...ChefAustin:chefaustin/lint-against-ruby-2.6.0

@ChefAustin
Copy link
Contributor Author

ChefAustin commented Mar 31, 2019

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants