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

Allow GetPermittedTriggers to Accept StronglyTyped Triggers with Guards in the Configurations #457

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

WahidBitar
Copy link

@WahidBitar WahidBitar commented Aug 15, 2021

There is no need to throw an exception when trying to get the Permitted Triggers. If it's not valid just don't return it.
The previous behavior was throwing exceptions when you have different types of strongly typed triggers and you try to check the available triggers for one of them.
I've added a Test Case to cover the issue. #450

…ed Triggers. If it's not valid just don't return it.
@anatoly-kryzhanovsky
Copy link

i dont think that exception flow is good for normal behaviour.
exception is a slow mechanism, so we need to avoid them if we can

check my pull request - https://github.com/dotnet-state-machine/stateless/pull/451/files

@WahidBitar
Copy link
Author

@anatoly-kryzhanovsky Thanks, I realize the performance issue about using exceptions. But I didn't want to change the logic in the internal implementation. I'm not quite sure about the internal implementation, are you sure that your changes will not affect the behavior of other places in the library?

@anatoly-kryzhanovsky
Copy link

in old behavior in case of type mismatch exception will be throw and method will exit. in my code i added check for that case, no internal method was changed

@WahidBitar
Copy link
Author

@tlk May you check this, please

@WahidBitar
Copy link
Author

please!

@crozone
Copy link
Collaborator

crozone commented Apr 12, 2023

Looks good. This does remove the laziness of the IEnumerable<TTrigger> by building the HashSet<TTrigger> immediately but I don't think that matters much at all.

I guess it's just a question of whether we go with this option or #451

@WahidBitar
Copy link
Author

@mclift & @p-m-j may you review this please.

@p-m-j
Copy link
Contributor

p-m-j commented Aug 2, 2024

@WahidBitar - I haven't really looked at or thought about this project in years so I don't think I'm the right person to review, I don't think I ever really deserved membership of the org.

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.

4 participants