-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Drop support for MRI 2.1 #5990
Drop support for MRI 2.1 #5990
Conversation
Gemfile
Outdated
@@ -6,8 +6,7 @@ gemspec | |||
|
|||
gem 'bump', require: false | |||
gem 'pry' | |||
# pry-byebug requires MRI 2.2.0 or higher. | |||
gem 'pry-byebug' if RUBY_ENGINE == 'ruby' && RUBY_VERSION >= '2.2.0' | |||
gem 'pry-byebug' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess you still need to check for MRI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it. 🙂
56bd7dd
to
2b0be32
Compare
I totally agree that dropping support for Ruby 2.1 form runtime, but I'm not sure we should drop it from the target ruby versions. We will be able to use new Ruby features by dropping support for old Rubies. I think it is a very big advantage. And RuboCop users can use RuboCop only with new Rubies. For example, users can disable RuboCop with Ruby 2.1 on Travis CI by the build matrix feature. However, personally I think dropping it from target ruby versions hardly has advantages for us. We can use new Ruby features without dropping. I got the same comment from a library owner when we dropped Ruby 2.0. I don't watch all issues of RuboCop recently because I'm busy. So sorry if you have discussed the comment already. |
It is a bit of a pet peeve on mine, having the community invest effort into enabling people to stay on old, insecure rubies, especially when upgrading Ruby (post 1.9) is relatively trivial. It's a disservice, if anything. However, I think it's ultimately a question about what RuboCop's official standpoint should be. Will defer to @bbatsov to make the decision. 🙂 |
Btw, technically speaking 2.2 has reached it's EOL as well. :-) I generally share @Drenmi's view on this, but I also realize that just keeping the parsing support around is not that big of a deal, so I'm fine with this. I'll leave this open for a few days to see what other users & contributors think and I'll make the decision then. (plus I want to cut a bugfix release in the mean time) |
Yeah. It's a bit unclear. I think the page might not be updated regularly, so it still says "scheduled for 2018-03-31". 😅 |
Yeah, that's a good point as well. OK, let's focus on 2.1 only then. :-) |
lib/rubocop/config.rb
Outdated
# 2.2 is the oldest officially supported Ruby version. | ||
DEFAULT_RUBY_VERSION = 2.2 | ||
KNOWN_RUBIES = [2.2, 2.3, 2.4, 2.5].freeze | ||
OBSOLETE_RUBIES = { 1.9 => '0.50', 2.0 => '0.50', 2.1 => 0.58 }.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe quote 0.58
too, for consistency?
The following is preparation for announcement about EOL of Ruby 2.2. Apart from that, I understand that this PR focuses on Ruby 2.1 :-) |
FWIW, I agree completely. Especially since rubocop encourages community standards / best practices. Even if this causes a little bit of friction, that's also what a lot of rubocop cops are designed to do in order to help push you in the right direction. |
Well, I've made a decision - let's just go with @Drenmi's original plan. If we manage to nudge more people to upgrade I think that's a good thing (tm) in the end of the day. |
2b0be32
to
a220ed0
Compare
@bbatsov Comments addressed, rebased, and pushed. |
@@ -17,6 +17,9 @@ | |||
* [#6006](https://github.com/bbatsov/rubocop/pull/6006): Remove `rake repl` task. ([@koic][]) | |||
|
|||
## 0.57.2 (2018-06-12) | |||
### Changes | |||
|
|||
* Drop support for MRI 2.1. ([@drenmi][]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a nitpick. I think that Changes
section should be after Bug fixes
section 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to reorder this. I don't have a strong preference on the subject, but ideally we should use a consistent ordering in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was wrong 💦
Since this item has not been released yet, it has moved to master (unreleased)
section with the following commit.
5f70fae
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build is failing because of a missing space. Ideally let's not push to master
. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry about that. I was not careful in pushing to master branch 😥
I confirmed that it was fixed by the following change.
a073d9a#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR18
As I changed changelog document only, I forgot to run bundle exec rake
. The push to master branch should be confirmed more properly locally than open a PR will checked with CI.
I thought that push to master branch was preferable, as far as minor changes (e.g. fix typo, cosmetic change) were concerned, rather than being notified to the watching users by opening a PR. Of course the contents to be reviewed opens a PR. (So, I will open a PR as a basis.)
If pushing to master branch, I pay close attention so that builds can be more securely passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry, @koic. Mistakes happen to everyone. I'm fine with people fixing small things directly on master
, I just want everyone to be very careful when doing this.
👍 |
Follow up of #5990 (comment). This commit added the address of pull request and fixed the position. Since it has not been released yet, it has moved to `master (unreleased)` section.
Ruby 2.1 will be dropped since next rubocop version rubocop/rubocop#5990
@@ -70,17 +70,6 @@ def some_method(foo, bar) | |||
end | |||
end | |||
|
|||
context 'when a required keyword argument is unused', ruby: 2.1 do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a late comment. Keyword arguments without defaults were introduced in Ruby 2.1, so I think this example is for Ruby >= 2.1, rather than Ruby < 2.2, i.e., it should be kept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I opened a PR #6104.
@@ -60,12 +60,5 @@ def meth(a, b, c, d: 1, e: 2) | |||
end | |||
RUBY | |||
end | |||
|
|||
it 'does not count keyword arguments without default values', ruby: 2.1 do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example should probably be kept, too.
This new version or RuboCop drops support for Ruby v2.1, so we have to do the same. See rubocop/rubocop#5990
Fowllow up of rubocop#5990. Since RuboCop 0.58.0 drops support for MRI 2.1. This PR drops minimum target Ruby version from `Lint/UnneededRequireStatement`.
Fowllow up of #5990. Since RuboCop 0.58.0 drops support for MRI 2.1. This PR drops minimum target Ruby version from `Lint/UnneededRequireStatement`.
Follow up rubocop#10632 (comment). Reverts part of rubocop#5990 and rubocop#6101. Only the Ruby version (2.1) to runtime should have been dropped, not code analysis. This PR makes Ruby 2.1 code analysis with `TargetRubyVersion: 2.1`. It aims to solve essentially the same problem as rubocop#10626, rubocop#10632, rubocop#10640, and rubocop#10644.
This new version or RuboCop drops support for Ruby v2.1, so we have to do the same. See rubocop/rubocop#5990
This new version or RuboCop drops support for Ruby v2.1, so we have to do the same. See rubocop/rubocop#5990
This new version or RuboCop drops support for Ruby v2.1, so we have to do the same. See rubocop/rubocop#5990
This new version or RuboCop drops support for Ruby v2.1, so we have to do the same. See rubocop/rubocop#5990
MRI 2.1 has been EOL for over one year. Time to drop official support? 🙂
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).and description in grammatically correct, complete sentences.
rake default
orrake parallel
. It executes all tests and RuboCop for itself, and generates the documentation.