-
Notifications
You must be signed in to change notification settings - Fork 94
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
types: Support Set and SetType #126
Conversation
42d6815
to
445c7b3
Compare
This initial copy does not include a real set implementation or handling for duplicate elements, however this sets the stage for expanding testing to cover the use case.
445c7b3
to
7a235c4
Compare
Provides basic element duplicate detection.
My upfront guess is that this implementation may need to change into allowing duplicates by overwriting existing elements during My first impression is that the current implementation may actually be desirable in this case (and allow the implementation to be much simpler) since practitioners would receive configuration validation errors for duplicate set elements and API fields this is being used against shouldn't have duplicates if this attribute type is being chosen. I'm not sure if its worth capturing this context anywhere (maybe on the Edit: As I reread this after submitting it, I'm not exactly sure how we could properly deduplicate only on |
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. Some small additions I recommended, and looks like some typos in some tests.
…t (Set).Validate() to be less O(n)^2
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.
Yeah, let's do it. Good work! 👍 🚀
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes #53