Skip to content
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

Statically typed selectors, take two. #993

Merged
merged 12 commits into from
May 28, 2020

Conversation

luleyleo
Copy link
Collaborator

The (hopefully) final part of #908 .

This time it just refactors Selector and Command.

@luleyleo luleyleo added S-needs-review waits for review breaking change pull request breaks user code labels May 26, 2020
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Show resolved Hide resolved
druid/src/command.rs Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/ext_event.rs Outdated Show resolved Hide resolved
@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels May 28, 2020
@luleyleo
Copy link
Collaborator 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)?
Maybe change it to "Refactored Command and Selector to be statically typed, similarly to Env and Key." to emphasis that there are more changes to it?

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 luleyleo added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 28, 2020
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// Convenience method for [`Command::new`] with this selector.
///
/// [`Command::new`]: struct.Command.html#method.new
pub fn carry(self, payload: T) -> Command {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this... command, maybe? or command_with_payload. Just feels a bit more discoverable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command seems fine but I'm thinking is there any value in using into_command given that it does a conversion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not, because it takes an argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and it would be to_, since self is Copy)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the look of with.

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if there's no payload? I guess it's fine if we pass a () and box it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that won't allocate anything, so no benefit in adding Option here.

druid/src/command.rs Outdated Show resolved Hide resolved
druid/src/command.rs Outdated Show resolved Hide resolved
@xStrom xStrom added S-waiting-on-author waits for changes from the submitter and removed S-needs-review waits for review labels May 28, 2020
@luleyleo
Copy link
Collaborator Author

I've renamed carry to with.

@luleyleo luleyleo added S-needs-review waits for review and removed S-waiting-on-author waits for changes from the submitter labels May 28, 2020
Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, very happy about this!

@luleyleo luleyleo merged commit 37877f7 into linebender:master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code S-needs-review waits for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants