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

Add support to Rails 7.1 #1110

Merged
merged 1 commit into from
Oct 14, 2023
Merged

Add support to Rails 7.1 #1110

merged 1 commit into from
Oct 14, 2023

Conversation

aovertus
Copy link
Contributor

@aovertus aovertus commented Oct 5, 2023

Update activerecord requirements to support Rails 7.1

@courtsimas
Copy link

@sampatbadhe @mbleigh what's the status on getting this merged?
Thanks in advance!

@tvongaza
Copy link

We've been running ActsAsTaggableOn with edge rails (updated to latest commit every week) for months without issue, now that edge rails is tagged at 7.1 our bundle install is breaking. Be great to have this merged in.

Follow up question, is the less than runtime dependency really required? Instead only checking the greater & equal to 6.0 condition? At very least consider changing it to something a bit more inclusive like '<= 7'.

@@ -22,7 +22,7 @@ Gem::Specification.new do |gem|
gem.post_install_message = File.read('UPGRADING.md')
end

gem.add_runtime_dependency 'activerecord', '>= 6.0', '< 7.1'
gem.add_runtime_dependency 'activerecord', '>= 6.0', '< 7.2'

Choose a reason for hiding this comment

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

Consider completely removing the less than condition in our runtime dependency.

Suggested change
gem.add_runtime_dependency 'activerecord', '>= 6.0', '< 7.2'
gem.add_runtime_dependency 'activerecord', '>= 6.0'

Further, consider using the Appraisal's gem to test compatibility with multiple versions of rails, including edge rails. A good example of this is the ActsAsTenant gem: https://github.com/ErwinM/acts_as_tenant/blob/master/Appraisals

Copy link
Contributor Author

@aovertus aovertus Oct 10, 2023

Choose a reason for hiding this comment

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

I've not check Appraisal yet but keep in mind that acts_as_taggable rely on ActiveRecord not Rails.

If I remember properly acts as tenant provide extension to more than just ActiveRecord and therefor requires Rails as a dependency not just one module

Choose a reason for hiding this comment

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

AFAIK Appraisal gem should work with any set of dependencies - rails just happens to be the most common use case. Maybe not needed here, but if there are concerns around which run time dependencies to support, the gem can different versions as different appraisal's to run the specs against.

CHANGELOG.md Outdated
* Features
* [@glampr Add support for prefix and suffix searches alongside previously supported containment (wildcard) searches](https://github.com/mbleigh/acts-as-taggable-on/pull/1082)
* [@donquxiote Add support for horizontally sharded databases](https://github.com/mbleigh/acts-as-taggable-on/pull/1079)
* [aovertus Add support for Rails 7.1](https://github.com/mbleigh/acts-as-taggable-on/pull/1110)

Choose a reason for hiding this comment

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

Suggested change
* [aovertus Add support for Rails 7.1](https://github.com/mbleigh/acts-as-taggable-on/pull/1110)
* [aovertus Add support for Rails 7.1 and Edge Rails by removing the less than (forward looking) runtime dependency check for current and future activerecord versions.](https://github.com/mbleigh/acts-as-taggable-on/pull/1110)

* Features
* [@glampr Add support for prefix and suffix searches alongside previously supported containment (wildcard) searches](https://github.com/mbleigh/acts-as-taggable-on/pull/1082)
* [@donquxiote Add support for horizontally sharded databases](https://github.com/mbleigh/acts-as-taggable-on/pull/1079)
* [aovertus Remove restriction around ActiveRecord 7.x versions allowing support until next major is released](https://github.com/mbleigh/acts-as-taggable-on/pull/1110)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tvongaza does this make sense ? I think we should not allow ActiveRecord 8 as we don't know which breaking changes will comes with it ?

Choose a reason for hiding this comment

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

Agreed, can do < 8 for now. Let's get this merged if possible please.

Choose a reason for hiding this comment

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

Looks good!

@tvongaza
Copy link

@seuros Any chance of doing a minor gem release to allow this gem to work with rails 7.1 & beyond?

@n-rodriguez
Copy link

Hi there! any news?

@seuros
Copy link
Collaborator

seuros commented Oct 14, 2023

OK i will now

@seuros seuros merged commit 10dd473 into mbleigh:master Oct 14, 2023
@n-rodriguez
Copy link

Thank you!

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.

None yet

7 participants