-
Notifications
You must be signed in to change notification settings - Fork 12
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
[WIP] Support Rails 7.0 #133
Conversation
@kbrock early call for help whenever you have time to look at the rails 7 failures or guide me on where to start... 🙇 |
ad1cc59
to
39107b0
Compare
I had started down this path in #125, but since this gem follows version matching with Rails, we decided to close that one. Should this be on a new separate branch? |
spec.add_runtime_dependency "activerecord", "~> 6.1.0" | ||
spec.add_runtime_dependency "activerecord", ">=6.1.7.6" |
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 this was the main sticking point in #125 and is why I closed it - in production this unlocks all versions of Rails, but this gem is intentionally not semver and matches the version of Rails that it supports directly. That is, this gem at this version can't (and shouldn't) work with versions >= 6.1.
In my PR I tried to make this part dynamic and only for test, but it turns out you really can't have "dynamic" code in a gemspec because it won't be packaged properly.
@@ -25,7 +25,7 @@ Gem::Specification.new do |spec| | |||
|
|||
spec.require_paths = ["lib"] | |||
|
|||
spec.add_runtime_dependency "activerecord", "~> 6.1.0" | |||
spec.add_runtime_dependency "activerecord", ">=6.1.7.6" |
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.
agreed, this needs to be:
spec.add_runtime_dependency "activerecord", ">=6.1.7.6" | |
spec.add_runtime_dependency "activerecord", "~>7.0.8" |
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.
and then also drop the 6.1 test suites.
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.
will do this once 6.1/7.0 get feature parity.
See rails commit: e3b9779cb701c63012bc1af007c71dc5a888d35a
6742cb8
to
1c5ab04
Compare
See: https://www.github.com/rails/rails/pull/42654 Note, Preloader and available records is a private API so it's not guaranteed to work as it currently does, see: https://www.github.com/rails/rails/issues/45804
Ugh. Joe. I just blew away this branch for you. |
update:
(still unsure if I should open another branch or just keep pushing here) |
update:
|
Checked commits jrafanie/activerecord-virtual_attributes@61aa955~...c8f525c with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint lib/active_record/virtual_attributes/virtual_fields.rb
|
This pull request is not mergeable. Please rebase and repush. |
This is being replaced by #150 |
Allow rails 7 gems in gemspec
see rails/rails#40776 for changes around the preloader