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

[suggestion] return instead of throw #77

Open
lofcz opened this issue Jan 18, 2021 · 3 comments
Open

[suggestion] return instead of throw #77

lofcz opened this issue Jan 18, 2021 · 3 comments

Comments

@lofcz
Copy link
Contributor

lofcz commented Jan 18, 2021

Now when invalid token / expression is encountered during evaluation throw statement is used to notify user something went wrong. In scenarios where EE is "proxied" behind some interface (for example a simple web application where I can create and evaluate my scripts) this is suboptimal behavior.

  • throw is expensive and shouldn't be used in scenarios where can be triggered often
  • it makes hard for users of EE to customize onerror behavior, texts of errors and recover from errors (forces try-catch)
  • there is no well defined list of recognized errors, users have to look them up in source code

This suggestion hence proposes that EE.ScriptEvaluate would return ExpressionEvaluatorResult insted of object. ExpressionEvaluatorResult is technically a tuple (could be reduced to a tuple) but for the sake of supporting pre-tuple versions of c# it would be nice to keep this as a class.

public class ExpressionEvaluatorResult {
  public object Result {get; set;} // <-- this is what ScriptEvaluate returns now
  public EvaluationResults State {get; set;} // <-- enum indicating result of evaluation
  public EvaluationResultErrorInfo ErrorInfo {get; set;} // <-- when "ok" this is null
}

Inner members definitions:

public enum EvaluationResults {
   Ok, // <-- first entry indicates the evaluation was successful
   MissingLParen, // <-- following entries all indicate an error
   MissingRParen
   ...
}

public class EvaluationResultErrorInfo {
   public int Line {get; set;} // <-- line in script where error occured
   public int Character {get; set;} // <-- character index in that line
   public string Message {get; set;} // <-- text we now pass to throw
}

Usage would then be:

ExpressionEvaluatorResult result = new ExpressionEvaluator("return 1 + 1;");
if (result.State == EvaluationResults.Ok) {
  // script evaluated successfully
  int ret = (int)result.Result;
}

Please note that all naming used is open for consideration and improvement, it should be as intuitive as possible and this is just from the top of my head.

@lofcz
Copy link
Contributor Author

lofcz commented Jan 21, 2021

@codingseb what do you think about this? Should you give this a go, I can prepare a PR. This is a breaking change but should be an easy one to migrate to.

@codingseb
Copy link
Owner

if you have A PR you can submit it.
But for this issue, I think we need a way to keep compatibility with Exceptions.
Could be an option and/or override of methods EE.Evaluate() and EE.ScriptEvaluate().
Also take note that a bunch of tests are relying on this.
Some projects too.

We also need to take care of the way it behave when Evaluate() or ScriptEvaluate() method are used in Expressions or scripts with respectively options OptionEvaluateFunctionActive and OptionScriptEvaluateFunctionActive.

lofcz added a commit to lofcz/ExpressionEvaluator that referenced this issue Jan 23, 2021
+ netstandard 2.0 -> 2.1 (to bump c# from 7.3 to 8.0)
@lofcz lofcz mentioned this issue Jan 23, 2021
@codingseb codingseb added this to the 1.5 milestone Feb 22, 2021
@TheoVC
Copy link

TheoVC commented Jun 28, 2021

I have done some tests on the use of try catch and is not more expensive or heavy.
by definition an exception should hardly ever happen. only if there was a syntax error ... but that shouldn't happen often

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants