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

Generate *_changed? and *_previously_changed? sigs for belongs_to relations #1955

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

cheshire137
Copy link
Contributor

Motivation

Models with a belongs_to relation now have a _changed? and _previously_changed? method since rails/rails#42751. This shipped in Rails 7, see https://github.com/rails/rails/blob/b84fa1083ca7e6422356fdf80cb33751d0a23eff/activerecord/CHANGELOG.md#rails-700alpha1-september-15-2021.

I noticed in my project Sorbet was giving me an error about my model not having a foo_changed? method when I had a belongs_to :foo, even though a respond_to?(:foo_changed?) returned true.

Implementation

I referenced existing _changed? method generation for regular columns on a model:

add_method(
klass,
"#{attribute_name}_changed?",
methods_to_add,
return_type: "T::Boolean",
parameters: [
create_kw_opt_param("from", type: setter_type, default: "T.unsafe(nil)"),
create_kw_opt_param("to", type: setter_type, default: "T.unsafe(nil)"),
],
)

While @erinnachen pointed out https://github.com/Shopify/tapioca/blob/main/lib/tapioca/dsl/compilers/active_record_associations.rb looked to be the compiler for methods coming from belongs_to relations, so I added the new method generation there.

I verified in the Rails implementation in https://github.com/rails/rails/pull/42751/files#diff-084ec489ba3b9f2e388734a99876f0d94776111278acfe4901235dca570040c5 that the new methods take no parameters.

Tests

I updated existing tests to expect the new method signatures to be present.

@cheshire137 cheshire137 requested a review from a team as a code owner July 11, 2024 17:25
@@ -206,6 +206,16 @@ def populate_single_assoc_getter_setter(klass, association_name, reflection)
"reset_#{association_name}",
return_type: "void",
)
if reflection.is_a?(ActiveRecord::Reflection::BelongsToReflection)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a good way of checking the version of Rails being targeted? We could avoid generating these signatures for pre-Rails 7 if so.

Copy link

Choose a reason for hiding this comment

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

I'm not sure if it's recommended but it looks like you could do this

Suggested change
if reflection.is_a?(ActiveRecord::Reflection::BelongsToReflection)
if reflection.is_a?(ActiveRecord::Reflection::BelongsToReflection) && Rails::VERSION::MAJOR >= 7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, thanks! I'll look around the repo, see if there are other such checks like this and, if so, I'll add this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, no such uses:

screenshot of VS Code search for Rails::VERSION

I do see a #rails_version used in tests, so I can investigate that.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use RubyGems API

if ::Gem::Requirement.new(">= 7.0").satisfied_by?(::ActiveJob.gem_version)

And in tests you can use the template and rails_version helpers

<% if rails_version(">= 7.0") %>

Copy link
Contributor Author

@cheshire137 cheshire137 Jul 12, 2024

Choose a reason for hiding this comment

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

That worked great, thank you!

@cheshire137
Copy link
Contributor Author

I have signed the CLA!

@cheshire137 cheshire137 marked this pull request as draft July 12, 2024 19:57
@cheshire137 cheshire137 marked this pull request as ready for review July 12, 2024 20:12
@cheshire137 cheshire137 marked this pull request as draft July 16, 2024 14:38
cheshire137 and others added 2 commits July 16, 2024 09:39
https: //github.com/Shopify/pull/1955#discussion_r1678141642
Co-Authored-By: Ufuk Kayserilioglu <[email protected]>
https: //github.com/Shopify/pull/1955#discussion_r1678141696
Co-Authored-By: Ufuk Kayserilioglu <[email protected]>
@cheshire137 cheshire137 marked this pull request as ready for review July 16, 2024 14:42
@paracycle paracycle added the enhancement New feature or request label Jul 16, 2024
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, this looks great!

@paracycle paracycle merged commit 429f1fb into Shopify:main Jul 16, 2024
16 of 17 checks passed
@cheshire137 cheshire137 deleted the dirty-relation-sig branch July 16, 2024 15:42
@NexusGamingHub
Copy link

HI Respected ma'am I am having a problem in getting my profile approved for the GitHub sponsorship and for that I was searching on the community discussion page for the answer and then I found one question or problem similar to me and you solved it then I check your profile and then I know that you are the staff of the GitHub so please view my profile and repository and please approve my profile for GitHub sponsorship please it will mean a lit for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants