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: ActiveRecord::QueryMethods::WhereChain#missing #596

Conversation

rhiroe
Copy link
Contributor

@rhiroe rhiroe commented Jun 11, 2024

Added ActiveRecord::QueryMethods::WhereChain#missing in version 6.1 or later.

Fixed to return ::ActiveRecord::QueryMethods::WhereChain when the where method has no argument.

I confirmed that no errors occurred when used with rbs-rails.

@rhiroe rhiroe force-pushed the add/activerecord-querymethods-wherechain-missing branch 5 times, most recently from 31bd3e6 to 6b26687 Compare June 11, 2024 08:47
@rhiroe rhiroe marked this pull request as ready for review June 11, 2024 08:53
Copy link

@rhiroe 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.

@rhiroe rhiroe force-pushed the add/activerecord-querymethods-wherechain-missing branch from 6b26687 to f34ee44 Compare June 11, 2024 09:07
@rhiroe rhiroe marked this pull request as draft June 11, 2024 09:18
Added `ActiveRecord::QueryMethods::WhereChain#missing` in version 6.1 or later.

Fixed to return `::ActiveRecord::QueryMethods::WhereChain` when the `where` method has no argument.
@rhiroe rhiroe force-pushed the add/activerecord-querymethods-wherechain-missing branch from f34ee44 to ccaa96f Compare June 11, 2024 09:33
@rhiroe rhiroe marked this pull request as ready for review June 11, 2024 09:35
Copy link

@rhiroe 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.


module QueryMethods
class WhereChain
def missing: (*Symbol associations) -> Relation
Copy link
Collaborator

Choose a reason for hiding this comment

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

This Relation is ::ActiveRecord::Relation.
So, the model information is missing.
Aren't you trying to get the model information from here?

How about like this?

module QueryMethods
  class WhereChain[Relation]
    def missing: (*Symbol associations) -> Relation
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try it.

@rhiroe rhiroe requested a review from ksss June 12, 2024 01:03
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.

Almost good 👍
Can you add testing code for missing?

@rhiroe
Copy link
Contributor Author

rhiroe commented Jun 12, 2024

If we mimic the RBS generated by rbs-rails, we can write it like this...

class User < ActiveRecord::Base
  extend _ActiveRecord_Relation_ClassMethods[User, ActiveRecord_Relation, Integer]

  class ActiveRecord_Relation < ::ActiveRecord::Relation
    include _ActiveRecord_Relation[User, Integer]
    include Enumerable[User]
  end

  class ActiveRecord_Associations_CollectionProxy < ::ActiveRecord::Associations::CollectionProxy
    include _ActiveRecord_Relation[User, Integer]
  end
end

Include or extend _ActiveRecord_Relation_ClassMethods and _ActiveRecord_Relation causes a large number of MethodDefinitionMissing errors.

# Type checking files:

............................................................................................................................................F................

test.rb:11:8: [error] Cannot find implementation of method `::Test::User.select`
│ Diagnostic ID: Ruby::MethodDefinitionMissing
│
└   class User < ApplicationRecord
          ~~~~

test.rb:11:8: [error] Cannot find implementation of method `::Test::User.find`
│ Diagnostic ID: Ruby::MethodDefinitionMissing
│
└   class User < ApplicationRecord
          ~~~~

......

Detected 65 problems from 1 file
No untyped calls detected. 🐾
 => Failure 🚨

# Failed gems
gems/activerecord/7.1

So I include or extend::ActiveRecord::Base::ClassMethods and ::ActiveRecord::Relation::Methods instead.

It looks like it was designed for that purpose.
375eca7

@rhiroe rhiroe requested a review from ksss June 12, 2024 07:21
@ksss
Copy link
Collaborator

ksss commented Jun 12, 2024

It is enough to show that missing is associated with some type, even if the code is not executable.
I do not mind if there is no code that mimics rbs_rails.

@rhiroe
Copy link
Contributor Author

rhiroe commented Jun 13, 2024

@ksss Sorry, I have a question.
Is it that I am not getting Approve from you because the current test is not sufficient?

@ksss
Copy link
Collaborator

ksss commented Jun 13, 2024

Yes. It seems to me that there is an increase in excessively managed test code.

The test code I had in mind is as follows:

User.where.missing.to_sql

@rhiroe
Copy link
Contributor Author

rhiroe commented Jun 14, 2024

I see. Thank you for your answer and example.

@rhiroe rhiroe force-pushed the add/activerecord-querymethods-wherechain-missing branch from c429445 to 0d03dd3 Compare June 14, 2024 01:06
@rhiroe rhiroe force-pushed the add/activerecord-querymethods-wherechain-missing branch from 0d03dd3 to 02ffe1e Compare June 14, 2024 01:08
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.

APPROVE

Copy link

Thanks for your review, @ksss!

@rhiroe, @ksss This PR is ready to be merged.
Just comment /merge to merge this PR.

@rhiroe
Copy link
Contributor Author

rhiroe commented Jun 14, 2024

/merge

@github-actions github-actions bot merged commit 14cafb8 into ruby:main Jun 14, 2024
4 checks passed
@rhiroe rhiroe deleted the add/activerecord-querymethods-wherechain-missing branch June 14, 2024 08:46
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.

2 participants