-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
Support for Ruby 2.4 through 2.7 #466
Conversation
- rvm: 2.3 | ||
gemfile: gemfiles/rails_6_0.gemfile | ||
- rvm: 2.4 | ||
gemfile: gemfiles/rails_4_2.gemfile | ||
- rvm: 2.4 | ||
gemfile: gemfiles/rails_6_0.gemfile | ||
- rvm: 2.5 |
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 think tests should run for ruby 2.5 and rails 4.2 combo as well
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 agree but tried to keep with the spirit of the set of exclusions that was already there.
Perhaps @luke-hill, who created this set in the first place, can explain the reasoning for not testing 4.2 on Ruby 2.5?
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.
At the time we originally drafted the 2.5 addition to travis (Or at least first started culling the travis releases), we made a conscious decision that the "newest" ruby at the time wouldn't support the oldest rails we supported. Because that Max/Min combo was perceived to be not desirable.
Also 2.5 (For another reason), was a Rails6 enforcement. So when we introduced rails6, it felt like most people on 2.5 would go straight to rails6 for big gains.
If we want to tweak which items are supported that's fine. But I definitely don't think we should introduce extra old versions, when we're already supporting quite a vast range of rails / ruby as it is.
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.
Right, so Rails 4.2 would only be tested with Ruby 2.4. This makes sense to me.
I'm going to mull over the possibility of inverting the logic here, because with so many exclusions I find it hard to have a clear overview of what will be built. I don't know if that's possible with Travis' config, but I'll see.
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 philosophy here is if you can justify the change that's fine. I'm not gonna rule with an iron fist here.
By default (With simple maths), we have 5*5 tested permutations. And I think Rails6 rules out the top2/3. So realistically by default we have like 22 permutations.
We need to get that down a little bit, but then I suppose it's also about volume. If we're not really firing many PR's at it, I'm happy to open it up a little bit, I just wouldn't want something previously excluded to be "reincluded" so to speak.
@mvz can we move this PR into ready for review? As someone else has come forward to do some follow-up work to this. |
@@ -20,6 +20,34 @@ Metrics/BlockLength: | |||
Style/NumericLiteralPrefix: | |||
EnforcedOctalStyle: zero_only | |||
|
|||
# Enable new cops |
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.
We can use a new style here called
AllCops:
NewCops: enable
Nested underneath the above.
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.
For what is worth, I prefer explicitly enabling cops that we're interested in (even if it's all of them), rather than blindly accepting every new from rubocop.
0c79892
to
54a0ef9
Compare
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.
Looks good to me!
Summary
Update the set of tested and supported Rubies to 2.4 - 2.7
Details
Motivation and Context
This relates to #455 and lowers the maintenance burden.
How Has This Been Tested?
Travis will have to check on all supported Rubies.
Types of changes
Not sure.
Checklist:
Not sure yet either.