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 a criteria method to the type adapter #591

Merged

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented Jan 4, 2021

No connected issue

After #587, there was a still one place with a hard-coded reference to the type extension namespaces

macro generate_criteria_method(query_class, name, type)
def \{{ name }}
column_name = "#{table_name}.\{{ name }}"
\{{ type }}::Lucky::Criteria(\{{ query_class }}, \{{ type }}).new(self, column_name)
end
end

This adds a criteria method that takes in the query and the column name and returns an instance of an Avram::Criteria for the given type.

One of the other benefits it gives us is that we don't need special handling for generic types since we are calling methods on the class instead of using it to access a namespace.

This would be a breaking change since the criteria method is completely new and custom types would need to implement it.

The rules for adding custom types would be:

  • The type must have an adapter class-level method that returns a class that includes the Avram::Type module
  • That class must have a criteria class-level method that returns an instance of Avram::Criteria for the type

I considered moving the criteria method onto the type we extend but I didn't want to pollute the core library types with another function.

@matthewmcgarvey matthewmcgarvey added the BREAKING CHANGE This will cause a breaking change label Jan 4, 2021
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

This is probably fine, just that one part confusing me a little

{% if column[:type].is_a?(Generic) %}
# Checking Array type
generate_criteria_method(BaseQuery, {{ column[:name] }}, {{ column[:type].type_vars.first }})
generate_criteria_method({{ column[:name] }}, {{ column[:type] }})
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused on why we don't need to check for Array(String) and it will know to use String.adapter 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Given a type of Array(String) it will call Array(String).adapter and that method calls .adapter on the generic type of the array which in this case is String.adapter.

class Array(T)
# Proxy to the `T`'s adapter so we can call methods like
# `Array(String).adapter.to_db(["test"])`
def self.adapter
T.adapter
end
end

Copy link
Member

Choose a reason for hiding this comment

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

ahh! boom! I missed that part.

@matthewmcgarvey matthewmcgarvey merged commit 6620951 into luckyframework:master Jan 6, 2021
@matthewmcgarvey matthewmcgarvey deleted the avram-criteria-method branch January 6, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This will cause a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants