Statically typed selectors, take two.#993
Statically typed selectors, take two.#993luleyleo merged 12 commits intolinebender:masterfrom luleyleo:typed-selectors-2
Conversation
luleyleo
added
S-needs-review
xStrom
left a comment
xStrom
left a comment
There was a problem hiding this comment.
Generally looks good. I do think we should be consistent about naming though. Using payload / argument / object to reference the same concept is too much. Let's pick one and go with that.
xStrom
added
S-waiting-on-author
|
@xStrom I think I've addressed all your points. What I'm not sure about yet: Should I add changelog entries for the methods I've added or renamed, or should it all be compressed to one, less specific entry (as it is now)? I did not rebase it yet, as it will break Github's review history, but if it is ok with you @xStrom , then I'll do the rebase. |
luleyleo
added
S-needs-review
xStrom
left a comment
xStrom
left a comment
There was a problem hiding this comment.
A few more nits. Also you can always rebase as far as I'm concerned, because I can still see the newer commits and only their changes.
Yeah let's change the changelog entry to be clearer about the signature changes.
Perhaps something like:
`Command` and `Selector` have been reworked and are now statically typed, similarly to `Env` and `Key`. ([#993] by [@finnerale])
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
Cool, I think this is a solid improvement and I'm happy you took it on!
My main nit is with carry; I think we can do better. I also have a few little doc suggestions you're welcome to ignore.
druid/src/command.rs
Outdated
There was a problem hiding this comment.
I would call this... command, maybe? or command_with_payload. Just feels a bit more discoverable.
There was a problem hiding this comment.
command seems fine but I'm thinking is there any value in using into_command given that it does a conversion?
There was a problem hiding this comment.
I think not, because it takes an argument.
There was a problem hiding this comment.
(and it would be to_, since self is Copy)
There was a problem hiding this comment.
How about just with(payload)?
I would like the name to stay rather short and with seems to read nicer than command.
submit_command(SHOW_SAVE_PANEL.carry(options));
submit_command(SHOW_SAVE_PANEL.with(options));
submit_command(SHOW_SAVE_PANEL.command(options));| let object: Option<Box<dyn Any>> = object.map(|obj| obj as Box<dyn Any>); | ||
| let object = object.map(|o| o.into()); | ||
| Command { selector, object } | ||
| pub(crate) fn from_ext(symbol: SelectorSymbol, payload: Box<dyn Any>) -> Self { |
There was a problem hiding this comment.
what if there's no payload? I guess it's fine if we pass a () and box it.
There was a problem hiding this comment.
Yes, that won't allocate anything, so no benefit in adding Option here.
xStrom
added
S-waiting-on-author
|
I've renamed |
luleyleo
added
S-needs-review
3 participants
The (hopefully) final part of #908 .
This time it just refactors
SelectorandCommand.