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

Update for rubocop ≥ 0.7x #49

Merged
merged 1 commit into from
Oct 1, 2019
Merged

Update for rubocop ≥ 0.7x #49

merged 1 commit into from
Oct 1, 2019

Conversation

jacobthemyth
Copy link
Contributor

@jacobthemyth jacobthemyth commented Sep 23, 2019

In preparation for moving some things to Ruby 2.7, we need to eventually use rubocop ≥ 0.7x so that the new language version is recognized by rubocop. This PR is my best effort to allow an incremental upgrade strategy without totally breaking backwards compatibility. My thought is that since rubocop 1.0 is nearing completion, this gem could continue to also be pre-1.0 and try to maintain backwards compatibility. However, my definition of backwards compatibility would be that upgrading to a new patch or minor version of rubocop-github should not introduce any new cop offenses. This might not be the best strategy since the rubocop authors have said that even though the API of rubocop itself will stabilize and follow semantic versioning after 1.0, the cops will continue to introduce "breaking changes" meaning that it will be allowed to introduce a new cop in a patch or minor version that is turned on by default so upgrading to these versions could introduce new offenses even after following semantic versioning (see rubocop/rubocop#5612). So, I'm happy to change this to be simpler if it makes sense to do so. FWIW, this may all seem like over-engineering, but because of #46 I figured I should make an attempt at backwards compatibility. Because the gem is pre 1.0, I think it would be valid and much simpler to just make the changes directly, so I'm happy/eager to simplify this PR if backwards compatibility is not required.

Changes

  • Extract configurations that are valid across the whole range of rubocops (0.59 - 0.7x) to config/_default_shared.yml and config/_rails_shared.yml. These files are intended to be inherited by higher level configs.
  • Create config/default_deprecated.yml. The only behavior of this file currently is to pull in config/_default_shared.yml and set DisabledByDefault to true. If there are breaking changes to cops, they could be put here in further upgrades. For instance, if the name of a cop is changed, the old name could be put in default_deprecated.yml and the new name could be put in default_edge.yml. Currently, all cop names are the same across the supported versions of rubocop, but this could change. The reasoning behind putting DisabledByDefault: true in the deprecated config is that it seems like a gem should not be so opinionated and leave this sort of global configuration to the client. It could break existing clients to remove it though, so I thought it should still be part of the default path.
  • Another reason to split out a "deprecated" config is so that it could require a Ruby file that prints a warning to stderr. I don't think it's appropriate to do that now, but I think the last version of rubocop-github before 1.0 could add this so that it could warn users to upgrade. My main motivation was allowing incremental upgrade, though, so I left out the warning message.
  • config/default.yml simply inherits config/default_deprecated.yml so as not to break any clients who require that file.
  • Perhaps most controversially, config/default_edge.yml is intended to be used on the latest version of rubocop in which the performance cops have been extracted to a gem. I left the performance cops in config/_default_shared.yml and required rubocop-performance in config/default_edge.yml even though the gem is not listed in the gemspec. Unfortunately, rubocop-rails and rubocop-performance both require a very recent version of rubocop so it would have prevented the incremental upgrade of rubocop.
  • These patterns were repeated for config/rails.yml as well.

@jacobthemyth
Copy link
Contributor Author

/cc @jhawthorn since it looks like you've worked in this project before.

Copy link
Contributor

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this cautious approach. We can always simplify this later.

@tenderlove tenderlove merged commit 6bc6dfb into github:master Oct 1, 2019
@jacobthemyth jacobthemyth deleted the ruby-2.7 branch October 1, 2019 20:45
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

Successfully merging this pull request may close these issues.

3 participants