Update Ribose OSS style guide for Rubocop 0.50, and clean it up a bit

[skalee]

Rubocop 0.50 renames plenty of cops. This pull request applies required updates. Also, duplicated settings are removed, and settings are ordered alphabetically with the following Ruby script:

path = "style/.rubocop.yml"
file = File.open path
text = File.read file
conf = text.strip.split /\n(?=\w)/
conf[2..-1] = conf[2..-1].sort!
file.reopen path, "w"
text = conf.join "\n"
text << "\n"
file.write text

[abunashir]

@skalee: Our ruby style guidelines are highly influenced by thoughtbot style guide and most of our style guidelines are the reflection of that guide. Looking at this pull request it's really hard given the number of changes, could you please point me to some specific changes?

[skalee]

There are no changes to the style guide actually, @abunashir. When I've tried the Ribose's config with Rubocop 0.50 I got several warnings and one fatal error, which forced me either to downgrade Rubocop or to update this config file. So I did the latter, precisely in commit c44c749. The other two ones are just clean-ups (removing duplications and reordering).

Changes are depicted in commit messages. I believe that reviewing commits one by one will be far easier than looking at the aggregated diff.

[skalee]

Actually, please hold on with this pull request. I found one quirk in my changes, and I need to investigate it.

[skalee]

That quirk was coming from incorrect cop name. The warning Rubocop was printing was hinting incorrect things. Anyway, fixed and rebased.

[skalee]

Another problem. It appears that Code Climate doesn't support Rubocop 0.50 yet. I guess it's better to wait a while with this pull request, and perhaps constraint Rubocop dependency in all gems to Rubocop < 0.50. I believe they'll fix it in the near future, so I'm not downgrading configs in my gems.

[skalee]

CodeClimate does support Rubocop 0.50 now, however it defaults to earlier versions. I can add CodeClimate configs if someone desires it.

[skalee]

There are few more issues with this one.

[skalee]

Every recent Rubocop release introduces some backwards-incompatible changes, and it's difficult to get a config which would work with different Rubocop versions. And unfortunately CodeClimate, whilst it allows to chose a Rubocop version, typically does not support the most recent one, contrary to Hound, which enforces the freshest without giving any option to change that. In the result, for the second time this month I am experiencing that either tool gets broken.

I suggest either disabling Rubocop checks in CodeClimate (it is enabled in some projects, e.g. uri_format_validator), or removing Hound from projects.

Your opinion, @ronaldtse?

[ronaldtse]

I'm fine either way -- the Rubocop version does not concern me that much.

@ribose-jeffreylau and @abunashir ?

[ribose-jeffreylau]

To me it seems houndci is too unpredictable. Like in
this issue, new PRs could be littered with warnings because of a new Rubocop version that can't be controlled.

[skalee]

Hound reports fatal errors only, like this one: riboseinc/uri_format_validator#83 (review). Regular warnings are not displayed.

There are about 7 releases of Rubocop every year. I don't mind upgrading Rubocop configs in this rate, especially that changes are often trivial. So let's do whatever you find more convenient.

[abunashir]

Sorry for the delay, let me try to explain my thought process about how I feel about this.

In regards to Hound and CodeClimate, both of these are amazing tools and both of those tools have some sort of limitations for the moment. Personally, I like the fact that there are some automated tools to ensure the style consistency and I think in that terms the Hound CI works great. It instantly adds a comment whenever there is a style violation and developer can see that as a review and possibly fix it.

But for codeclimate, I could not feel the same way, but it does a pretty good job to check code complexity and duplications, and sometimes it evens start to complain/fail the PR that might not need instant refactoring.

So, most of the case my approach is to have the Hound CI enabled for every PR so it will comment whenever there is a violation, and have the codeclimate enabled but disable it to update any PR (need a change in setting) and when it will start the count a lower grade in the master branch then do the necessary refactoring.

Please let me know what do you this about this approach and I will also post another comment soon about rubocob configuration. cc: @skalee, @ronaldtse, @ribose-jeffreylau.

[skalee]

@abunashir In CodeClimate, Rubocop is purely optional, and disabled by default. Complexity checks (at least some of them) do not require Rubocop, as it can be seen in CodeClimate reports for synced_resources gem which uses default CodeClimate configuration.

[abunashir]

@skalee: In regards to the codeclimate: As long as it does not affect and provide strange results for code complexity and duplication than I am fine with that, and I also don't mind having codeclimate silently reporting those so periodically we can check and then update our code.

[abunashir]

In regards to the rubocop: The main reason behind having our custom rubocop is to provide a baseline (on top of default rubocop config) so we can start any of the Ruby projects with that one as starting point and then customize it for each project based on their requirements.

But to be perfectly honest, we don't want to completely manage this script by our own as there might be lots of maintenance work in the long run, so the plan was to adopt some of the available ones that are close to our development practices, and if we don't agree with some rules then override those

And in regards to the pull request, I really appreciate your effort and you really brought some of the important facts here, but I am just afraid that once we start to change/organize it than the actual script then it might be quite difficult when the actual script gets updated (in this case thoughtbot style guide). But I totally agree that this script should not break our suite and we should update it as frequently as possible, in fact thoughtbot already updated their script for 0.50, so it's time to do another update.

This pull request also makes me realize, that we should probably minimize this as much as possible, and I think we are using the one from the hound repo (which is outdated and I think we did that for some compatibility issue on hound). We just need to double check with the latest style guide and hound compatibility and if that works then just pull that one to this repo and if we have any customization then add those at the end so we can easily track those in the future.

[skalee]

@abunashir Rubocop supports configuration inheritance. How about separating their defaults from our overrides? If we store the unmodified Thoughtbot's defaults in a file .rubocop.thoughbot.yml, then the upgrade process will be as simple as replacing the whole content. Also, configuration can be inherited from a remote URL, so the whole upgrade process can be limited to obtaining a new permalink. I prefer storing it in a local file, though.

[abunashir]

@skalee: Sounds great, I am 👍 for separating the thoughtbot's rubocop and our local one. Last time when I checked Hound CI was acting writed with inheritance script, but that's been a while and that's something we can re-check and if that has been fixed in the recent release or not.

I also used the latest script (minimal rules) on the ribose-ruby repo and it seems to be working fine, so if you guys can have a look on that one and we are okay with everything then we can also pull the latest one from thoughtbot repo, Thanks.

[skalee]

@abunashir I have different observations.

1. I've took Thoughtbot's config and Ribose's config, and run diff to compare them. In order to eliminate possible false positives, I've removed all metadata like Description or StyleGuide from both configs. Moreover, I've ensured that ordering of configuration sections is consistent in both files. Despite that, diff is 800 lines long. Such big difference cannot be explained with any of small commits made to this file: https://github.com/thoughtbot/guides/commits/master/style/ruby/.rubocop.yml.

2. I've tested projects I participate in against that freshest Thoughtbot's config, and there are differences in output. Moreover, some are important ones. For instance, the Thoughtbot's config disables checks for maximum method length, whereas we enforce 10 lines maximum.

The amount of discrepancies leads me to suspicion that actually Ribose's Rubocop config is not based on Thoughtbot's guide, at least not one that one you linked in your comment. Maybe they used to keep older versions elsewhere? I don't know.

Also, it raises a question how to separate Ribose's additions from the base document, whatever it is. Diff is unmanageable, so if you got that list of Ribose's additions stashed somewhere, or can extract it from some other projects, it would be really helpful.

But if the list of Ribose's additions cannot be determined easily, then I guess we can keep Ribose's settings to metrics cops (only about 10 items), and switch to Thoughtbot's defaults for everything else. That should result with some configuration we are able to manage.

[abunashir]

@skalee: Yes, you are right and as I mentioned in the previous comment, the current rubocop.yml we have is the one from the hound repo conifg and we did that for hound ci compatibility issue. It was pretty much identical with hound ci compatibility.

But the recently proposed one, should be identical with thoughbot's style guide (with additional include section) and it also seemed to compatible with hound ci now, that's why I asked for some thoughts on pulling that one as our updated config.

As of now, we did not do that much of a customization from the actual script, once we pull that changes then would we require this pull request? Do you have any strong opinion on some of the rules on the recently proposed ones (thought's one) that might be applicable to any type of ruby project?

[skalee]

Thanks for clarification, @abunashir! Somehow I've misinterpreted your previous comment.

[skalee]

@abunashir I've just confirmed that Hound indeed supports configuration inheritance: riboseinc/attr_masker#56. I guess I can split it now for easier maintenance. I'm fine with basing Ribose's style guide on Thoughbot's one. FYI the most current TB's guide results with a warning (unrecognized cop Lint/LiteralInCondition) after cop rename in 0.51, but it's not a big deal IMO.

So, just to sum things up and confirm everything, we are generally moving from Hound's Rubocop configuration to ThoughtBot's one, correct?

What do we do with metrics settings then? TB's one is very relaxed on this topic, e.g. maximum allowed method length in TB's is hundred lines. Should we enforce our limits here? (Not necessarily as strict as in Hound where it's only ten lines.)

Alternatively, we can stay with Hound's config which is actively maintained. The most current version is fully compatible with Rubocop 0.51.

[abunashir]

@skalee: Thanks for the confirmation, and yes, we are moving from our current rubocop config to the thoughtbot one. I think we all already spent quite some time on this issues by now, so if everything is okay can we close this issue now?

Regarding any other questions, could you please ping me on ribose or skype, so I can try to understand better and maybe explain if possible and hopefully that will save all of us sometime :)

[skalee]

@abunashir Thanks for clarification. I'll leave my questions here to keep the discussion in one place.

Firstly, we can inherit TB configuration in several ways.

1. From our local copy, saved let's say to .rubocop.tb.yml.
2. From remote url, specific commit, e.g. https://raw.githubusercontent.com/thoughtbot/guides/91509c26b3f84beebb239592561e41a77fdd0fa6/style/ruby/.rubocop.yml
3. From remote url, master branch HEAD: https://raw.githubusercontent.com/thoughtbot/guides/master/style/ruby/.rubocop.yml

The last option seems the most convenient as it's auto-updating. On the other hand, options 1 and 2 enable us to to track version compatible with specific Rubocop version. Any recommendations? My opinion is to go with 2 (unless given project does not constrain Rubocop version in its gemspec).

Secondly, the TB's configuration is far more permissive in code metrics than Hound's. Do we want to keep the current settings that maximum allowed method or block length is 10 lines?

[ronaldtse]

@skalee how about we just based off TB's configuration, and write out the overrides in this repo? I think we should keep minimal maintenance of this. We could also use Travis to tell us if (and what) the base has changed. Thanks!

[skalee]

@ronaldtse This is what I want we want to do, but there are questions on details.

[skalee]

Okay then, I have rewritten the guide. Few things to note:

• I have created a .rubocop.ribose.yml which inherits Thoughtbot's guide and includes our few additions (basically metrics cops configuration).
• The Thoughtbot's guide is referenced by a particular commit (thoughtbot/guides@91509c2). We can change that to "master" to enable auto-updating.
• The brand new .rubocop.yml includes nothing but few settings which are likely to be changed in projects, and a reference to GitHub-hosted copy of .rubocop.ribose.yml.
• Contrary to .rubocop.yml, .rubocop.ribose.yml is not supposed to be copied to projects. Therefore, I am thinking about creating examples directory and moving all files which are supposed to be copied to projects there (currently .rubocop.yml and .hound.yml).
• I have created couple of tags: compat-rubocop-0_48 and compat-rubocop-0_51. They will make referencing older configurations easier.
• I need to update README slightly, will do when @abunashir approves my changes.

[skalee]

Actually, I suggest following directory structure for this repository:

<ROOT>
  - /examples (files which should be copied to new projects)
    - /ruby (files which should be copied to new Ruby projects)
      - .rubocop.yml
      - .hound.yml
      - .gitignore (we don't have it now in our guides but we should add it)
      - .editorconfig (same story)
    - … (for other languages)
  - /config (various configuration which should stay hosted)
    - .rubocop.ribose.yml
  - README (and so on)

[abunashir]

@skalee: In the suggested approach new project will include rubocop config that inherits the remote oss repo, and remote oss cofig inherits remote tb's config, it's two external HTTP calls to run rubocop and my concern here:

• Locally rubocop does some caching, but how does that gonna work in a container environment, do two requests each time it runs rubocop?

• Compared to a local copy, how does that impact on performance? considering running it multiple times in a container env, a slow remote resource and limited internet availability?

• Is there a full support for our standard tools and CI?

[ronaldtse]

I feel that the remote requests aren't a big issue, assuming that GitHub is mostly available to fetch the rubocop configuration. That said we could always create a task in this repo to "localize" the final, desired, rubocop config?

[abunashir]

Yap @ronaldtse, I was considering the same, maybe do a rake task / script, that will combine both of our and thoughtbot's config content into a rubocop file and periodically, we run the tasks to update our configs that merges and update our rubocop, that way we do not have any remote dependencies.

[skalee]

@abunashir, regarding your concerns:

• Locally rubocop does some caching, but how does that gonna work in a container environment, do two requests each time it runs rubocop?

• Compared to a local copy, how does that impact on performance? considering running it multiple times in a container env, a slow remote resource and limited internet availability?

• Is there a full support for our standard tools and CI?

Hound is the only tool affected. We are not going to run Rubocop in Travis or CodeCov for obvious reasons. Neither we are going to run Rubocop in CodeClimate due to possible Rubocop version incompatibilities.

Hound fetches remote configuration correctly, as it was tested in this pull request: https://github.com/riboseinc/attr_masker/pull/56/files#diff-11a0d7906801d9dea0eccb85667ad811. It is very likely that Hound performs two additional requests every time, however performance penalty is insignificant. Hound is network bound due to how it works: first it fetches the source code from GitHub, then it pushes comments to GitHub. Adding another step (fetching configuration from GitHub) shouldn't make things much worse. Furthermore, from my observations, Hound is the fastest among our checking tools, so there's no risk that we'll have to wait for it with merging.

When it comes to using Rubocop locally, the remote configuration is cached, so there should be no issues even when developer's computer is offline.

I can make a tool for combining configuration from other sources, but personally, I consider it an overkill. At least at current stage.

The main disadvantage of inheriting configuration from remote source is that it gets cached in some ugly-named dot-prefixed file, e.g. .rubocop-https---raw-githubusercontent-com-thoughtbot-guides-91509c26b3f84beebb239592561e41a77fdd0fa6-style-ruby--rubocop-yml. It needs to be added to GitIgnore, and excluded from project in editor. But that's manageable.

[ronaldtse]

Thanks @skalee , looks like we should just merge this!

[abunashir]

Let's merge it 👍

[skalee]

Follow-up in #8.