Skip to content

Remove CanExecute checks from Execute logic #131

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 9, 2022
Merged

Remove CanExecute checks from Execute logic #131

merged 2 commits into from
Mar 9, 2022

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Mar 3, 2022 •
edited
Loading

This PR removes the CanExecute checks from the Execute methods of all command types.

Video of this working as expected on UWP:

devenv_mnwhKo25tc.mp4

@Sergio0694 Sergio0694 added improvements ✨ Improvements to an existing functionality introduce breaking changes 💥 This change would be a breaking change need documentation 📃 This change needs a related documentation PR next preview ✈️ This changes will be available in the upcoming preview optimization ☄ Performance or memory usage improvements priority 🚩 An issue or change that has priority mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit labels Mar 3, 2022
@Sergio0694 Sergio0694 marked this pull request as ready for review March 4, 2022 14:54
@Sergio0694 Sergio0694 force-pushed the dev/remove-can-execute-checks branch from 3dc198f to ea701b4 Compare March 6, 2022 16:36
Arlodotexe
Arlodotexe reviewed Mar 8, 2022
View reviewed changes
Copy link
Member

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

Maybe I missed it, but it seems like the expected usage is that the user must check CanExecute before calling Execute. Do we have tests that assert the expected behavior when the user doesn't do that?

@Sergio0694
Copy link
Member Author

Yup, there's some tests for both command types that check the execution is performed when skipping checks 🙂

@Sergio0694 Sergio0694 force-pushed the dev/remove-can-execute-checks branch from ea701b4 to 3daad89 Compare March 9, 2022 21:14
@Sergio0694 Sergio0694 merged commit 4f148cf into main Mar 9, 2022
@delete-merged-branch delete-merged-branch bot deleted the dev/remove-can-execute-checks branch March 9, 2022 21:33
@Tederean Tederean mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Arlodotexe Arlodotexe Arlodotexe approved these changes

Assignees

No one assigned

Labels

improvements ✨ Improvements to an existing functionality introduce breaking changes 💥 This change would be a breaking change mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit need documentation 📃 This change needs a related documentation PR next preview ✈️ This changes will be available in the upcoming preview optimization ☄ Performance or memory usage improvements priority 🚩 An issue or change that has priority

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Sergio0694 @Arlodotexe