rubocop / rubocop

A Ruby static code analyzer and formatter, based on the community Ruby style guide.
https://docs.rubocop.org
MIT License
12.61k stars 3.05k forks source link

Migration between deprecated cop rules sucks #2783

Closed ptarjan closed 8 years ago

ptarjan commented 8 years ago

How should I deal with having 100s of engineers needing to upgrade in unison? (Most are using editor integration so just using bundle exec rubocop won't work).

Right now I have this in my config:

Style/TrailingComma:
  EnforcedStyleForMultiline: consistent_comma

And I need to have

Style/TrailingCommaInLiteral:
  EnforcedStyleForMultiline: consistent_comma
Style/TrailingCommaInArguments:
  EnforcedStyleForMultiline: consistent_comma

If I put the new options in now, they spew but the old version works. Great. But when someone uses the new version it stops and emits:

Error: The `Style/TrailingComma` cop no longer exists. Please use `Style/TrailingCommaInLiteral` and/or `Style/TrailingCommaInArguments` instead.
(obsolete configuration found in .rubocop.yml, please update it)

If I leave the old config only then the new version pukes with the same. How the heck do I migrate this for everyone at my company?

maxjacobson commented 8 years ago

If I'm understanding you correctly, the source of the issue is that the .rubocop.yml file is not included in the codebase, but the editor integration points to some global configuration file. Is that right? If so, I recommend including the config file in the repo and changing it in lock-step with the rubocop version. See bundler for an example of this.

ptarjan commented 8 years ago

@maxjacobson no, my .rubocop.yml is in the codebase. The problem is, if I update the config file to move to the new options, I can't leave the old option in the file. So once I do the change, people on old rubocops will lose trailing comma linting.

maxjacobson commented 8 years ago

I see. But the version of RuboCop which is running isn't based on a Gemfile in the project when it's using an editor integration? How is that working?

ptarjan commented 8 years ago

bundle install in its wisdom will install things globally. So when I run rubocop from the command line, I just run the highest version gem of any project that has rubocop in its Gemfile.

maxjacobson commented 8 years ago

Yea, for that reason I generally don't run rubocop with rubocop. Rather, I run it with bin/rubocop.

(As long as RuboCop is in your Gemfile, you can run bundle binstubs rubocop to generate some stub executables which respect your Gemfile)

jawshooah commented 8 years ago

Wouldn't bundle exec rubocop accomplish the same thing, minus one step?

ptarjan commented 8 years ago

Right. But the editor integrations by default don't. And getting my whole company to change their default configs isn't too fun.

So back to the talks at hand, can we make it not fatal for deprecated rules please? Or at least for a given deprecation window?

Sent from my iPhone

On Feb 4, 2016, at 6:58 PM, Josh Hagins notifications@github.com wrote:

Wouldn't bundle exec rubocop accomplish the same thing, minus one step?

— Reply to this email directly or view it on GitHub.

alexdowad commented 8 years ago

What is the editor integration you are using? If it doesn't allow you to run a specific version of the gem, that is a serious deficiency.

We can consider having deprecation windows and so on, but that doesn't really fix the problem. Sooner or later, we will have to break backwards compatibility. When we do, you will have the same problem. The real problem here is that you haven't found a good way to run a specific version of RC from your editor integration.

ptarjan commented 8 years ago

I'm perfectly happy breaking stuff, I just want a migration window where I can yell at everyone and say "please upgrade or it breaks in a month".

We have a heterogenous mixture of editors. Vim, Emacs, Sublime and Rubymine are the prominent ones.

Maybe we can put something in the .rubocop.yml indicating the minimum needed version and do something that shows up nicely in editors if you don't meet that with then gem install instruction would be nice? (Above and beyond the migration window, which is higher priority I feel). How do other linters deal with this?

alexdowad commented 8 years ago

The README states:

RuboCop's development is moving at a very rapid pace and there are often backward-incompatible changes between minor releases (since we haven't reached version 1.0 yet). To prevent an unwanted RuboCop update you might want to use a conservative version locking in your Gemfile:

gem 'rubocop', '~> 0.36.0', require: false

I would suggest that you even consider locking to a specific version using = 0.36.0 or the like.

ptarjan commented 8 years ago

I do lock to a specific version my Gemfile, it is just that editor integrations don't stick to that. I think they just use rubocop from the path, and not bundle exec rubocop. :(

alexdowad commented 8 years ago

Can you just uninstall the unwanted gems?

ptarjan commented 8 years ago

It is less the unwanted gems, it is more the fact that if I go into my repo and set the Gemfile to 0.37.0, then everyone's editors don't magically get that format. Instead they start spewing since they are on 0.34.2 and the config file now has 0.37.0 options in it. And I can't leave the old 0.34.2 options in there, since people that DO update, will have their rubocop just stop working since it hard fails on old options.

ptarjan commented 8 years ago

All I want is for the old rule failure to be soft not hard for a version.

alexdowad commented 8 years ago

So update the Gemfile and .rubocop.yml in the same commit. Everybody who pulls that commit runs bundle and then they are happy.

jawshooah commented 8 years ago

@alexdowad The problem is that the editor integrations apparently don't all respect the Gemfile.

While there is certainly no semver-driven compulsion to retain backwards compatibility pre-1.0, there are obviously a ton of people already using RuboCop in production. Perhaps it wouldn't be terrible to deprecate obsolete options for a version before removing them in the next.

alexdowad commented 8 years ago

The problem is that the editor integrations apparently don't all respect the Gemfile.

Even if they just run whatever rubocop binstub happens to be on your PATH, I believe that should be the newest version installed.

ptarjan commented 8 years ago

@alexdowad Right. So that means neither option is good:

1) I can't leave the old option in the rubocop.yml file because then the new version breaks on its presence 2) People will have to all in unison rebase all their old branches once I rev the main Gemfile. That's a huge PITA for the whole eng org.

alexdowad commented 8 years ago

Perhaps it wouldn't be terrible to deprecate obsolete options for a version before removing them in the next.

Sure, nobody will argue that it's _un_desirable to display deprecation notices in advance. Unfortunately, it also takes work and planning. There are a lot of other things to do on this project, and we don't have an excess of developers, nor do the current developers have an excess of free time.

RC is moving quickly towards the first stable release, but before we get there, more breaking changes are ahead. If we have to keep a bunch of unwanted code around to maintain compatibility, that will slow us down. (Just an opinion!)

Note, too, that giving deprecation windows for old cops will not eliminate breaking changes. How about changes in the format of the console output? The fact is that all kinds of changes are ahead. Some of them will break backwards compatibility.

That is why the README recommends (maybe not strongly enough) that everyone who needs stability should run a specific version of RC. When a new version comes out, you can test it at your leisure and upgrade if it works well for you.

It doesn't matter if you run it from a CLI, a GUI, an editor plugin, a git commit hook; whether you invoke it programmatically, like Hound; whether you do so in an office, or on an airplane, an automobile, or the back of a camel, climbing up the snowy slopes of Mt. Hermon. You must know what version(s) you have, and you must select the version you want to run. If you can't do that, don't expect things to work.

The real problem in this issue is not breaking changes between versions. The problem is users not having control over what version of the software they are using.

This problem is not unique to RC. There are many situations where a person might want to use different versions of the same program on the same device. Ever heard of rvm and rbenv? Good example.

"But I'm using editor integration!"

For the sake of argument, let's pretend that out of the hundreds of engineers referred to in this thread, there is not one who can fix those editor plugins to make them run a specific RC. Clone the code, grep for rubocop, change it to bundle exec rubocop, recompile? Nah. Maybe the plugins are written in some incredibly arcane fashion; maybe they're coded in the machine language for some obsolete Russian mainframe from the Sputnik era, and run on an emulator written in binary lambda calculus. Anyways, we're not touching them.

So we're stuck. Right? ...Wrong.

But just how wrong is it? First, try this pop quiz:

Pop quiz: What is the "magic trick" that allows rvm to switch you from using one Ruby interpreter to another?

Answer: it simply switches the PATH env variable in your shell. Can you use the same trick to control which rubocop your editor runs? Yes!

On *nix systems, when a program is run via fork+exec, the exec/execve/etc. syscall takes a list of environment variables/values and gives them to the new process. When exec'ing another program, most programs just pass their own env variables through. So if you set the PATH in your shell, all the processes launched from the shell, even indirectly, will generally receive that same PATH.

When invoking other programs (like rubocop), some programs spawn a subshell, and run a command in it; others directly fork+exec. In either case, the PATH is generally respected. (I just tried stracing ruby to see what it does when you run a shell command in backticks. It doesn't use a subshell; but you can see it doing a bunch of stat syscalls to search its PATH.)

In short, if you launch your editor with a specially tweaked PATH, without doing anything else, you can control which rubocop it will run. There are many ways of doing this. One way is to create a little wrapper script which sets the PATH and then runs the editor. If you launch your editor from some fancy graphical menu or something, there should be a way to set the environment variables which it will be launched with.

For our masses of unwashed users who are not familiar with such fancy tricks, it would be good if the editor plugins out there can be fixed. But that's not a battle I am ready to fight. I have my hands full already.

ptarjan commented 8 years ago

You clearly feel very passionate about your stance on this, so I'll leave it be. Thank you for the wonderful piece of software.

I would recommend you consult the other prominent linters (ESLint is the one I'm most familiar with) once you revisit this idea for rule migration patterns.

alexdowad commented 8 years ago

@ptarjan Thank you. What is the status of the problem your team is having with using RC editor integration?

ptarjan commented 8 years ago

@alexdowad I upgraded our internal config and Gemfile to 0.37.0 and sent out a company wide email telling everyone they need to upgrade and rebase all their old PRs before using rubocop.

I'm just going to stay oncall for master breakages this week due to missing commas.

rud commented 6 years ago

There's a tool for this now: https://github.com/pocke/mry