-
-
Notifications
You must be signed in to change notification settings - Fork 810
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 Rails 5.0, Rails 5.1, and Rails 5.2.0 support #1073
Drop Rails 5.0, Rails 5.1, and Rails 5.2.0 support #1073
Conversation
3093037
to
571e773
Compare
Note that, in the case of Rails 5.0, we were not even testing it, so it might be broken already. |
I also dropped support for Rails 5.2.0. In my opinion, it doesn't provide any benefit to explicitly support different bug fix releases in the same series. If a bug fix in Rails causes a regression in ransack, it's simpler to bump the minimum required rails version to the latest patch level release. |
ab26968
to
46c3ba6
Compare
Since they have reached their End of Life.
They should be the same as ransack's.
Instead, require a minimum of Rails 5.2.1.
46c3ba6
to
49dde38
Compare
@deivid-rodriguez thanks for all of your contributions for Ransack. It's definitely time to drop 5.0 and 5.1 support, but 5.2 is still supported by Rails and there are probably many users still on 5.2 Is it possible to restrict this PR to 5.0 & 5.1? |
Hi! Actually, the correct policy is this one: https://edgeguides.rubyonrails.org/maintenance_policy.html. The link you posted should've been updated since the release of 6.0.1 according to rails/rails#37058 (comment), not sure why that didn't happen. |
Also, although I probably didn't explain it right, I'm not dropping support for all 5.2.x releases, I'm only dropping support for 5.2.0. So 5.2.1, 5.2.2, 5.2.3 and any further security releases in the 5.2 series are still suported. The rationale is that a bug in Rails 5.2.0 forced ransack to maintain custom code just to support Rails 5.2.0. In this case, in my humble opinion, it's simpler to bump the minimum required version to the version of Rails that fixed the bug instead of having to maintain custom code to support a specific patch level version of Rails that had a bug affecting ransack. |
That's much clearer, thanks @deivid-rodriguez ! I'll try to get your various PRs merged over the weekend, including this one. |
Let me know if any updates or rebases are needed, I think there'll be some conflicts. |
@deivid-rodriguez Does this project use semver? If so, dropping support for older Rubies and Rails versions should have been a minor release at a minimum as peeps won’t expect breaking changes with a patch release. |
Hi! I don't know whether this project uses semver or not. Also, it's unclear where "dropping support for language versions" fall inside semver. My personal take is to not consider it a breaking change and ship those kind of changes on minor releases. I'm interested how you got bit by this, though. If you're using a ruby version for which support was dropped, |
But this was a patch release.
And I haven’t in terms a breakage. I only stumbled into it by reviewing the changes and was surprised that such a change was a patch release. You are right about bundler though, it should not resolve to the new version on unsupported language/Rails versions. |
Yeah, I know, that's why I said "my personal take". I'm not sure whether doing this in a patch release was a mistake, or maybe an interpretation that supporting rubies/rails that are no longer maintained and that could have un-patched vulnerabilities is a bug 😄 Anyways, I don't think this is a big deal. |
Since they have reached their End of Life.