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 type definitions to delete_all #713

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kazuyainoue0124
Copy link

Fix types definitions for delete_all methods.
Changed the return type of delete_all methods to Integer.

Copy link

@kazuyainoue0124 Thanks for your contribution!

Please follow the instructions below for each change.
See also: https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md

Available commands

You can use the following commands by commenting on this PR.

  • /merge: Merge this PR if CI passes

activerecord

You changed RBS files for an existing gem.
You need to get approval from the reviewers of this gem.

@hibariya, @ksss, @Little-Rubyist, please review this pull request.
If this change is acceptable, please make a review comment including APPROVE from here.
Screen Shot 2024-03-19 at 14 13 36

After that, the PR author or the reviewers can merge this PR.
Just comment /merge to merge this PR.

@tk0miya
Copy link
Contributor

tk0miya commented Oct 31, 2024

FYI: API doc mentions the return value of #delete_all.

Returns the number of rows affected.
https://api.rubyonrails.org/classes/ActiveRecord/Relation.html#method-i-delete_all

Copy link
Collaborator

@ksss ksss left a comment

Choose a reason for hiding this comment

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

How was the effect of this change?

Perhaps gems/activerecord/6.0/activerecord.rbs also needs to be modified for substantial effect.

@tk0miya
Copy link
Contributor

tk0miya commented Nov 18, 2024

I noticed ActiveRecord::Associations::CollectionProxy#delete_all takes an optional argument dependent. But ActiveRecord::Associations::Relation#delete_all does not take arguments at all.

But the current design of rbs_rails expects both to be the same.
Therefore it's not good to modify definitions under activerecord.rbs (ActiveRecord::Relation::Methods, ActiveRecord::Base::ClassMethods, _ActiveRecord_Relation and _ActiveRecord_Relation_ClassMethods).

It's better to change only the return types of these methods under activerecord.rbs.
And we need to hack rbs_rails to override the argument type of CollectionProxy#delete_all via rbs_rails like pocke/rbs_rails#289 does.

Refs:

@kazuyainoue0124
Copy link
Author

Thank you for your valuable feedback.
I apologize for not fully understanding the differences between ActiveRecord::Associations::CollectionProxy#delete_all and ActiveRecord::Associations::Relation#delete_all, as well as the specifics of rbs_rails.

In this pull request, I have only updated the return types.
I will address the override of CollectionProxy#delete_all in rbs_rails separately.

If there are any remaining issues, please let me know.

@ksss
Copy link
Collaborator

ksss commented Dec 12, 2024

Sorry for the delay in responding.

  • When modifying 6.0/activerecord-generated.rbs, move as far as possible to 6.0/activerecord.rbs.
  • I would like at least one line of testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants