Skip to content

Conversation

Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Mar 3, 2022

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

2 participants