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

Add CascadeIfDirty property to make optimization opt-in #3732

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

rockfordlhotka
Copy link
Member

Fixes #3721

@rockfordlhotka
Copy link
Member Author

@lwmorris067 and @hurcane - here's the PR for my proposed fix to the breaking change around running rules.

@hurcane
Copy link

hurcane commented Mar 14, 2024

This appears to restore the original non-performant behavior by default, while still implementing the slightly optimized behavior that @mtavares628 can take advantage of.

I would still vote for the changes proposed in discussion #3720, but I understand the motivation to implement this now because it's holding back the next release.

If the other proposal were to be implemented, the CascadeIfDirty option wouldn't really be needed, would it?

@rockfordlhotka
Copy link
Member Author

rockfordlhotka commented Mar 14, 2024

@hurcane I remain open to your idea, but need to see more specifics. I'm unlikely to have time to implement/test it in the next 10-14 days due to work/travel.

Update - I see your wonderfully detailed post with details, thank you!! I've captured it in #3736

Copy link

@mtavares628 mtavares628 left a comment

Choose a reason for hiding this comment

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

This looks good to me. I do think that @hurcane's #3736 concept may be the more efficient way to go in the end (or even a combination of both of these), but just taking a look at the code that is already there it seems like it may require some re-work of CheckRulesForProperty because it doesn't currently expose the list of the rule's DirtyProperties, only the AffectedProperties.

@rockfordlhotka rockfordlhotka merged commit 368b8a3 into MarimerLLC:main Mar 14, 2024
1 check passed
@rockfordlhotka rockfordlhotka deleted the 3721-rules branch March 14, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance rule execution behavior to minimize breaking change impact
3 participants