-
Notifications
You must be signed in to change notification settings - Fork 27
Relational class method additions #45
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
base: master
Are you sure you want to change the base?
Relational class method additions #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work so far! Seems like a crucial feature for this gem.
My biggest issue is, though, that you're proposing to rely on a protected method. This is potentially not a great long-term solution.
I'm also wondering, if you can test other scoped relations. Current active/inactive is pretty streight forward. But what if try to scope arrayed columns? What if scoped will be some Arel expression? Would be awesome to see if that works.
@@ -14,7 +14,6 @@ Gem::Specification.new do |spec| | |||
|
|||
spec.files = `git ls-files -z`.split("\x0") | |||
spec.executables = spec.files.grep(%r{^bin/}) { |f| File.basename(f) } | |||
spec.test_files = spec.files.grep(%r{^(test|spec|features)/}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can you please leave tests files? I do tend to use bundle open
occasionally and it's nice to see how code is being used. Overhead of having tests with gem is not that big.
@@ -47,6 +48,7 @@ def create_database | |||
t.integer :codes, array: true, default: [] | |||
t.citext :roles, array: true, default: [] | |||
t.uuid :references, array: true, default: [] | |||
t.boolean :active, default: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we not add another field to model and use any of existing fields? Just to keep code as lean as possible.
# Remove the STI inheritance type from the outer query since it is in the subquery | ||
unscope(where: :type).from(subquery_scope).pluck(:tag) | ||
# Handles the unique case of prepending method with "where("tag like ?", "aws%")" | ||
missing_like_tag_prepend = current_scope&.where_clause&.send(:predicates)&.none? { |pred| pred.to_s.include?("tag like") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: we're using a protected :predicates
here which is internal to AR and could change at any moment. This rubs me in a wrong way... as a code that potentially could cause issues.
I've been brainstorming for a solution without private/protected methods and only managed to come up with a following:
missing_like_tag_prepend = current_scope&.where_clause&.send(:predicates)&.none? { |pred| pred.to_s.include?("tag like") } | |
missing_like_tag_prepend = !current_scope&.where_clause.any? { |t| t.is_a?(String) && t.include?("tag like") } | |
I would greatly appreciate if you can add CHANGELOG entry as well with this pr. |
This PR adds additional functionality allowing the following class methods the ability to handle scoped relations calling the class method, it handles the specific exception for the "tag like" prepend, and adds testing for the scoped relation logic