ruby / gem_rbs_collection

A collection of RBS for gems.
MIT License
255 stars 106 forks source link

add: ActiveRecord::QueryMethods::WhereChain#missing #596

Closed rhiroe closed 3 months ago

rhiroe commented 3 months ago

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.

github-actions[bot] commented 3 months ago

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


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.

github-actions[bot] commented 3 months ago

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


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 commented 3 months ago

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. https://github.com/ruby/gem_rbs_collection/commit/375eca7206281228ea413d754036d0362faadb61

ksss commented 3 months ago

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 commented 3 months ago

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

ksss commented 3 months ago

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 commented 3 months ago

I see. Thank you for your answer and example.

github-actions[bot] commented 3 months ago

Thanks for your review, @ksss!

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

rhiroe commented 3 months ago

/merge