-
-
Notifications
You must be signed in to change notification settings - Fork 910
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 comment about no support of validate_uniqueness_of(:item).scoped_to(array) #1355
Conversation
# NOTE: This gem does not fully support testing uniqueness scoped to an | ||
# array of associations. For a reference thread, please check this issue | ||
# https://github.com/thoughtbot/shoulda-matchers/issues/814 | ||
# |
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.
# NOTE: This gem does not fully support testing uniqueness scoped to an | |
# array of associations. For a reference thread, please check this issue | |
# https://github.com/thoughtbot/shoulda-matchers/issues/814 | |
# | |
# Support for testing uniqueness validation scoped to an association is not available. | |
# For more information, please refer to https://github.com/thoughtbot/shoulda-matchers/issues/814 |
I think we can get rid of gem
word as its contextual. Also i think array is not needed.
Does this change makes sense?
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.
Hey @KapilSachdev I completely agree with the first part about removing gem
. But the array part is actually the point of this PR. We can validate scoped to an association (using attribute name, not object), but when an object is uniquely scoped to more than one object, we can't really validate it. It is possible to test with scope to a list of attributes, but it is unclear how this would behave in multiple scenarios.
The solutions proposed on #814 we not as simple as I thought it could be when I first created the issue, and manually testing it (without shoulda) would be easier to read. Because of this, it was decided to add this specific comment to make clear this limitation.
I'll change the text and update PR. Thanks for suggestion
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 good to me! Thank you!
Closes #814