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

Make scopes available for abstract classes #1250

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

Teots
Copy link
Contributor

@Teots Teots commented Oct 28, 2022

Motivation

At the moment sorbet can't figure out that classes, that are marked as abstract, have access to scopes. The following example illustrates this:

class A < ApplicationRecord
  extend T::Sig
  abstract_class = true

  scope :deleted, -> { where.not(deleted_at: nil) }

  class << self
    extend T::Sig

    sig { returns(Integer) }
    def count_deleted
        # Method `deleted` does not exist on `T.class_of(A)`
        deleted.count
    end
  end
end

Since those scopes can be used, I think it makes sense to generate the rbi files accordingly.

Implementation

I removed the filter for abstract classes in ActiveRecordScope and did the same thing in ActiveRecordRelations to make the generated relation methods work.

Tests

Tests for ActiveRecordScope have been extended.

@Teots Teots requested a review from a team as a code owner October 28, 2022 14:44
@Teots Teots changed the title Include abstract classes Make scopes available for abstract classes Oct 28, 2022
@Teots Teots force-pushed the include_abstract_classes_in_active_record_scope branch from af838b3 to 2f072ae Compare October 28, 2022 14:48
@paracycle
Copy link
Member

did the same thing in ActiveRecordRelations to make the generated relation methods work.

Can't see this part in the PR. Is that missing or did you decide against this?

@Teots Teots force-pushed the include_abstract_classes_in_active_record_scope branch from 2f072ae to 3572410 Compare November 1, 2022 08:18
@Teots
Copy link
Contributor Author

Teots commented Nov 1, 2022

@paracycle Sorry, looks like I forgot to push this commit 🙈 It's there now.

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.

Looks good to me! We will have to check this against Core to make sure that we don't get any unwanted behaviour before merging though.

lib/tapioca/dsl/compilers/active_record_relations.rb Outdated Show resolved Hide resolved
@antn
Copy link

antn commented Jan 12, 2023

Ah, I just ran into this too. @paracycle, did this end up having unexpected behavior, or is it maybe good to merge? 😄

@paracycle paracycle force-pushed the include_abstract_classes_in_active_record_scope branch from 5b87ff3 to e64d47c Compare February 9, 2023 22:29
Teots and others added 2 commits February 9, 2023 23:58
Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
@paracycle paracycle force-pushed the include_abstract_classes_in_active_record_scope branch from e64d47c to 531eaca Compare February 9, 2023 23:58
@paracycle
Copy link
Member

Ok, I just had a chance to test this against Shopify Core and it seems to work. However, I removed the commit that was making AR relations to consider abstract classes as well, since that's not related to scopes and I don't think we would want folks to write relations off of abstract classes.

@paracycle paracycle merged commit 54898e4 into main Feb 10, 2023
@paracycle paracycle deleted the include_abstract_classes_in_active_record_scope branch February 10, 2023 00:17
@shopify-shipit shopify-shipit bot temporarily deployed to production February 21, 2023 12:01 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants