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

Rails 7 support #272

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Rails 7 support #272

merged 3 commits into from
Jan 18, 2023

Conversation

flavorjones
Copy link
Collaborator

@flavorjones flavorjones commented Jan 16, 2023

Alternative implementation to #267 that doesn't monkeypatch ActiveRecord.

This implementation subclasses ActiveRecord::Reflection::BelongsToReflection and overrides the compute_class method to allow ActiveHash associations.

I've also cherry-picked some relevant commits from #267.

@flavorjones flavorjones marked this pull request as ready for review January 16, 2023 20:22
@kbrock
Copy link
Collaborator

kbrock commented Jan 17, 2023

@flavorjones This looks great. A few thoughts:

  • Will this require developers to mixin this belongsto module everywhere for this functionality? (I vaguely remember this module in the past)
  • If so, we need to make sure this is a major version change with incompatibility note.
  • Do we need to document this change so it is easy for developers to fix when upgrading to 7.0?

(as with everywhere in rubyland. thanks for the contribution)

Copy link
Collaborator

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM. just minor logistics questions

@flavorjones
Copy link
Collaborator Author

@kbrock Thanks for the review and for asking these questions.

This should "just work" for folks who have followed the README and are extending their AR classes with ActiveHash::Associations::ActiveRecordExtensions (see this section in the README).

However, it looks like not all the tests were following that advice but were still passing with Rails 6 (see the change in this PR to spec/active_hash/base_spec.rb), which seems suspicious and makes me believe there may be apps out there which are "working" but will break with Rails 7. Maybe I'm not fully understanding Active Hash and how it's supposed to be used? I could use some advice, honestly.

@flavorjones
Copy link
Collaborator Author

Looks like the ActiveRecordExtensions module was added in v0.9.6 (see 153c58e) but wasn't added to this test (probably because it wasn't failing).

@kbrock
Copy link
Collaborator

kbrock commented Jan 18, 2023

Thanks.
I thought I remembered this module from a number of years ago.

Ok, I change my view from breaking change to bug fix.

@kbrock kbrock merged commit a4ea0c3 into master Jan 18, 2023
@kbrock kbrock deleted the flavorjones-rails-7-support branch January 18, 2023 20:09
@flavorjones
Copy link
Collaborator Author

@kbrock I just saw now that I still had a "wip" message in that last commit, urk, sorry, I should have fixed that. Candidly, I did not expect that code to pass on every Rails version and assumed I'd be coming back to clean something up. 🤷

@kbrock
Copy link
Collaborator

kbrock commented Jan 19, 2023

@flavorjones ugh. very sorry that I did not see that. ping me to get that through

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

3 participants