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

feat: consistently define fetch_multi_by #1468

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

cribbles
Copy link
Contributor

@cribbles cribbles commented Apr 6, 2023

Motivation

Following Shopify/identity_cache#534, IdentityCache will define fetch_multi_by for cache_index invocations regardless of the number of keys included in the index. (Previously only single-index keys were supported.)

We should lift the field_length restriction accordingly.

Implementation

Removes the field_length == 1 restriction (as well as the field_length variable designation, which is now useless).

Tests

Nothing changed.

Updated a unit test implicitly testing this behavior in 64983d4.

@cribbles cribbles requested a review from a team as a code owner April 6, 2023 13:36
@dylanahsmith
Copy link

My inclination would have been to remove any tests related to the field length restriction, tbh, but there weren't any, and I see no reason to extend tests beyond the happy path in this case. I am open to changing this if a suitable approach is proposed in the reviews.

Looks like there was a test for the absence of this generated method that can be changed to expect its presence. The test failure is

          it generates methods for combined cache_indexes                 FAIL (0.84s)
        --- expected
        +++ actual
        @@ -20,6 +20,9 @@
             sig { params(index_values: T::Enumerable[T.untyped], includes: T.untyped).returns(T::Array[::Post]) }
             def fetch_multi_by_title(index_values, includes: nil); end
         
        +    sig { params(index_values: T::Enumerable[T.untyped], includes: T.untyped).returns(T::Array[::Post]) }
        +    def fetch_multi_by_title_and_review_date(index_values, includes: nil); end
        +
             sig { params(keys: T::Enumerable[T.untyped]).returns(T::Array[::Integer]) }
             def fetch_multi_id_by_title(keys); end
           end
        /home/runner/work/tapioca/tapioca/spec/tapioca/dsl/compilers/identity_cache_spec.rb:223:in `block (3 levels) in <class:IdentityCacheSpec>'

@cribbles
Copy link
Contributor Author

cribbles commented Apr 6, 2023

@dylanahsmith d'oh. Corrected in 64983d4

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.

Thank you!

@cribbles cribbles merged commit a431979 into main Apr 6, 2023
@cribbles cribbles deleted the cribbles/consistently-define-fetch-multi-by branch April 6, 2023 15:51
@shopify-shipit shopify-shipit bot temporarily deployed to production April 12, 2023 17:19 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants