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

Relational class method additions #45

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

syrashid
Copy link

@syrashid syrashid commented Aug 4, 2024

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

  • "all_#{tag_name}"
  • "#{tag_name}_cloud"

Copy link
Collaborator

@skatkov skatkov left a 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)/})
Copy link
Collaborator

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
Copy link
Collaborator

@skatkov skatkov Aug 9, 2024

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") }
Copy link
Collaborator

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:

Suggested change
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") }

@skatkov
Copy link
Collaborator

skatkov commented Aug 10, 2024

I would greatly appreciate if you can add CHANGELOG entry as well with this pr.

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.

2 participants