-
Notifications
You must be signed in to change notification settings - Fork 164
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 multi-field scoping for slugs #274
add multi-field scoping for slugs #274
Conversation
mikekosulin
commented
Oct 20, 2023
- Add option for multi-field scoping in slugs, expanding beyond single-field/relation scopes without altering existing functionality
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.
Looks great! Let's simplify the code a bit with a uniform Array(...).each ...
6ede5de
to
47ff69e
Compare
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.
Much better, but the implementation is still a little confusing. See below. Thanks for hanging in here with me!
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.
I think version.rb is the last thing.
LMK if the nit makes sense, otherwise I can merge.
@johnnyshields any comments/issues? |
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.
LGTM! Thanks. I'll wait for @johnnyshields to chime in, will merge this week if we don't hear from him.
Hey @dblock, @johnnyshields any updates on merging? 😊 |
README.md
Outdated
end | ||
``` | ||
|
||
Note: this approach creates multiple indexes, differing from single-field scoping, and impacting database performance and storage. |
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.
Hmmm... shouldn't this still create only one compound index? For example:
slug :name, scope: %i[company_id department_id]
# Should create
index company_id: 1, department_id: 1, _slugs: 1
# NOT
index company_id: 1, _slugs: 1
index department_id: 1, _slugs: 1
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.
Sorry for the late response. You're right, it creates only a compound index in this case. I've fixed it here and forgot to remove the note.
@mikekosulin see comment |
@mikekosulin great work, merged! Thanks. |