-
Notifications
You must be signed in to change notification settings - Fork 394
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
crate.select()
support
#1977
crate.select()
support
#1977
Conversation
d635592
to
07e4722
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.
Seems pretty awesome! Can you add some tests/examples?
b37785a
to
139ff20
Compare
crate.annotation()
SelectList/SelectDict supportcrate.select()
support
ee5606f
to
37a6ed2
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.
This looks great! I haven't looked thoroughly at the tests in the starlark/select_* files yet, but otherwise this is looking really solid and pretty much ready to merge.
The comments I've labelled "very non-blocking" are "let's talk about whether in the future we should think about this" questions, and definitely not "let's do this in this PR" questions :)
37a6ed2
to
bdfe9bf
Compare
I've hopefully addressed most things here. Unfortunately hit a few things that need your input. Feel free to make any changes you consider necessary to merge, or I can make them tomorrow 🙂 |
543835a
to
cde8309
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.
LGTM, thanks!
Has been re-implemented and now has good test coverage
… SelectStringDict.
…alue|List|Set|Dict]`.
…t::items()` calls.
…et<T>>`) and changed how `unmapped` is grouped.
cde8309
to
becf50a
Compare
Updates
crate.annotation()
to supportcrate.select()
.Also includes a bunch of hopefully desirable cleanups, like removing the
serialize_starlark()
workaround that was no longer necessary.