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

Move Target inside Command. #1185

Merged
merged 4 commits into from
Sep 8, 2020

Conversation

luleyleo
Copy link
Collaborator

@luleyleo luleyleo commented Sep 6, 2020

This moves the Target into Command.

Currently, widgets receiving commands can't tell what the commands target actually was, and internally we have a similar problem, because we have to manually work with (Target, Command) tuples when ever we need access to the target.

Now Command::target can be used to access it at any time.

@cmyr If I remember correctly, you proposed this? I think it will work well.

@luleyleo luleyleo added S-needs-review waits for review breaking change pull request breaks user code labels Sep 6, 2020
@cmyr cmyr self-requested a review September 7, 2020 14:31
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.

Honestly, I like this a lot; in particular I like how it more explicitly communicates how commands work, and brings the concept of a target to the foreground.

I have some sort of back-of-mind concerns about whether this reduces flexibility in some ways, like having a single command that a container sends to all of its children (what is the target?) but I don't think this is a serious problem in practice.

@cmyr cmyr added S-ready PR is ready to merge and removed S-needs-review waits for review labels Sep 8, 2020
@luleyleo
Copy link
Collaborator Author

luleyleo commented Sep 8, 2020

Glad to hear. Once the checks passed, I'll merge it.

I have some sort of back-of-mind concerns about whether this reduces flexibility in some ways, like having a single command that a container sends to all of its children (what is the target?) but I don't think this is a serious problem in practice.

I don't think that will be an issue. You can still change a command's target at any time, if you want to.

@luleyleo luleyleo merged commit 9911137 into linebender:master Sep 8, 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-ready PR is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants