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

[Short-Circuiting The Evaluation While Parsing] #638

Merged
merged 7 commits into from
Jan 18, 2025

Conversation

pbhal
Copy link
Contributor

@pbhal pbhal commented Jan 18, 2025

This PR aims at allowing expression parsing for dynamic evalutaion.

@pbhal pbhal self-assigned this Jan 18, 2025
@pbhal pbhal enabled auto-merge (squash) January 18, 2025 11:12
@pbhal pbhal disabled auto-merge January 18, 2025 11:22
@pbhal pbhal merged commit 4f0c86c into main Jan 18, 2025
3 of 4 checks passed
@pbhal pbhal deleted the users/pbhal/dynamicParsing branch January 18, 2025 11:22
@flotoz
Copy link

flotoz commented Jan 31, 2025

Hi,
Could you explain why you haven't implemented an EnableException parameter or EnableExceptionAsErrorMessage value from ReSettings for example?
I'm experiencing a regression in my business rule related to exception handling.
This issue is caused by the global catch of ParseException in this implementation.

The global catch is interfering with my expected exception behavior, specifically:

// Act
Action act = () => ConditionInterpretor.Evaluate(expressionToEvaluate);

// Assert
act.Should().ThrowExactly<ParseException>().WithMessage("Unknown identifier 'A'");

How can I have the same behaviour as before ?

Thanks,
Florian

@pbhal
Copy link
Contributor Author

pbhal commented Feb 25, 2025

@flotoz - Valid point. We were evaluating it in terms of whether any parameters which is missing or is short-circuited should evaluate to false, but yes this makes more sense to have a toggle enabled/disabled for exception cascading further. We will keep the default value of this toggle to be true, so that the default behaviour of the library persists forward.
We will be releasing a new version this week. Thanks!

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.

2 participants