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

Extend cop metadata #5978

Closed
bbatsov opened this Issue Jun 9, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 9, 2018

Each cop should gain the following additional metadata:

  • VersionAdded - the RuboCop version in which it was added
  • VersionChanged - the RuboCop version in which it was changed in a user-impacting way (new config, updated defaults, etc)
  • VersionRemoved - the RuboCop version in which the cop was removed for whatever reason.

Those will be pretty useful for the documentation (so the manual generation has to be enhanced to include them) and keeping track of changes. Adding the fields is easy - the real work is going to be populating this metadata for existing cops (I guess this will happen gradually).

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Jun 9, 2018

Oh, I forgot the most important metadata.

  • Safe (true/false) - indicates whether the cop can yield false positives (by design) or not.
  • SafeAutoCorrect (true/false) - indicates whether the auto-correct the cop does is safe (equivalent) by design.

We'll use those two pieces of metadata to add options to run only safe cops or only safe auto-corrections. Again the real work here is just going through all the cops to fill the data in. Guess we can just assume that by default everything is safe unless marked otherwise to reduce the amount of work needed here.

@To1ne

This comment has been minimized.

Copy link

To1ne commented Jun 10, 2018

This one sounds interesting, and doable.

Also, it shouldn’t be that hard to write a script to find theVersionAdded.

How would so consider handling cases where you need multiple VersionChanged?

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Jun 10, 2018

I think in practice we need just the last one.

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Sep 5, 2018

@rubocop-hq/rubocop-core @Darhazer Our volunteer for this just pulled out of it and it's probably the most important ticket we have to resolve in the direction of RuboCop 1.0 (as it blocks everything related to using this metadata). Would one of you like to tackle this? The task is pretty simple and you don't really need to dig through all the versions - it'd be fine if those are filled just for the last release and fallback to nil until someone can do a bit of archeology.

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Sep 5, 2018

Same with the Safe data - we just need to put the support for it, fallback to "everything's safe" and fill in the data as we go.

And update the manual generation script, of course.

@Darhazer

This comment has been minimized.

Copy link
Member

Darhazer commented Sep 5, 2018

I'll take on this

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Sep 5, 2018

Thanks, Max! Much appreciated!

@Darhazer

This comment has been minimized.

Copy link
Member

Darhazer commented Sep 6, 2018

I've done a bit of archeology and have some questions:

  • Not all cops are listed in default.yml - it inherits from enabled.yml and disabled.yml, where all cops are listed. So either all should be moved in the default (and enabled and disabled would be somehow obsolete), or the metadata can be added to those base configs instead. WDYT?
  • I guess adding or removing autocorrect counts towards VersionChanged.
  • What about renaming cops and or/moving them to other departments? Is that a VersionChanged or VersionRemoved for the old name and VersionAdded for the new one?
  • Same goes for splitting cops in two - in that case, I guess it's really VersionRemoved for the original one and VersionAdded for the 2 new cops.
  • I guess changing enabled by default (both for the cop or its autocorrect) counts towards VersionChanged.
@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Sep 6, 2018

Not all cops are listed in default.yml - it inherits from enabled.yml and disabled.yml, where all cops are listed.

It only contains cops that have custom configuration options.

So either all should be moved in the default (and enabled and disabled would be somehow obsolete), or the metadata can be added to those base configs instead. WDYT?

Very much in favour. The split configuration was confusing to me at first, and keeps annoying me on a regular basis. 😬

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Sep 7, 2018

Not all cops are listed in default.yml - it inherits from enabled.yml and disabled.yml, where all cops are listed. So either all should be moved in the default (and enabled and disabled would be somehow obsolete), or the metadata can be added to those base configs instead. WDYT?

All should be moved to default. The enabled/disabled files are just remnants of the very first days of the project where cops didn't have any config/metadata and those files were the way to figure out if some cop should be enabled or not. We should have removed them a very long time ago...

I guess adding or removing autocorrect counts towards VersionChanged.

Yes. Any config changes count towards it.

What about renaming cops and or/moving them to other departments? Is that a VersionChanged or VersionRemoved for the old name and VersionAdded for the new one?

If something was truly renamed (like we've done in the past) it should be just VersionAdded for the renamed cop. After the metadata is introduced it will be VersionRemoved for the old cop and VersionAdded for the new one.

Same goes for splitting cops in two - in that case, I guess it's really VersionRemoved for the original one and VersionAdded for the 2 new cops.

Yes.

I guess changing enabled by default (both for the cop or its autocorrect) counts towards VersionChanged.

Yes.

@Drenmi

This comment has been minimized.

Copy link
Collaborator

Drenmi commented Sep 27, 2018

Amazing work @Darhazer! 🙇 There's another small remnant still there, I think. We used to have a few SafeMode flags, which probably were a failed attempt at introducing the Safe flag. Perhaps we should replace instances of this with the new flag?

maxh added a commit to maxh/rubocop that referenced this issue Oct 8, 2018

@ilyakatz

This comment has been minimized.

Copy link

ilyakatz commented Oct 17, 2018

I just tried the latest default config with the latest version of rubocop, and I'm getting a bunch of warnings

Warning: unrecognized parameter Bundler/DuplicatedGem:VersionAdded found in .rubocop.yml
% rubocop -v                                                                                                                                                                      
0.59.2

Any ideas on why this is happening?

@bbatsov

This comment has been minimized.

Copy link
Collaborator Author

bbatsov commented Oct 17, 2018

@ilyakatz Because the new metadata as added after 0.59.2. :-) There's no released version that supports it yet - 0.60 will be the first one when we cut it.

@ilyakatz

This comment has been minimized.

Copy link

ilyakatz commented Oct 17, 2018

ha, got it, i jumped them gun. will wait for it to come out. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment