Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Merged by Bors] - Query::get_unique #1263
[Merged by Bors] - Query::get_unique #1263
Changes from 4 commits
85ced1e
573f293
326dea9
93f07e3
964bfcf
4f577ab
370c044
a83531e
aa42a63
5af71fb
cc2f5c9
4bc8f0c
435a7f2
0be6c8c
11c0606
e0c7e36
06cd9cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 would say this is a good use of this api
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.
In general however, I think constraining uses like this not ideal. For example, the current version would trivially extend to having multiple fields with the same controls (to allow something like those things where people play multiple pokemon games with one set of inputs)
Although on the other hand, for this simple example this is probably fine.
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 this is sort of the crux of my hesitance to adopt get_unique(). It encourages people to design around "single entity" patterns. Maybe thats a good thing, but its a paradigm shift with implications we need to consider.
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 expect that the cases where we don't mind this pattern would be for rendering
Resources
, i.e. it's just for getting data from resources onto the screen, which I've called out as a sensible use above. That is, it's a logic error to have multiple scoreboard texts but a single Score resourceBut this case is just using that API for the sake of using it, where there's not actually an especially compelling reason to.
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 I agree. In this case, there might be a future "breakout++" where multiple balls come into play. And I think that for most things even if there is only one item we should encourage
query.iter()
. Imoget_unique()
should be reserved for cases that are truly game breaking.