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

Eval during parsing #37

Open
Vlek opened this issue Feb 21, 2021 · 2 comments
Open

Eval during parsing #37

Vlek opened this issue Feb 21, 2021 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@Vlek
Copy link
Owner

Vlek commented Feb 21, 2021

As stated in #3, there was another way that we could've handled the evaluation that would have allowed us to uncouple from click. I propose that we use that method now with pyparsing as during the parsing step one can give a function for how to handle the parsed portions as they are parsed. This allows us to move away from the messy evaluate function. As it stands currently, it's not pretty to look at. There are parts that have to be deeply couched and the typechecks are worrisome, especially for the functions as the operators are currently mostly x: Union[int, float], y: Union[int, float], but that isn't the case for all and there are nested if-statements to ensure types before it uses them. Still, mypy is unhappy with this set up.

Pros:

  • Type checking should be happy and the errors that mypy are raising should go away.
  • This should also end up being cleaner code as lambdas that call each operation specifically should be much easier to grok.

Cons:

  • Requires a rewrite of a substantial amount of the diceparser, including a lot of filling out for the EvaluationResults class that will need a lot of operation overloading to make everything pretty.
  • This method I do not believe will be compatible with the new parser that is introduced in 3.10 which we are considering in Change parser to new Structural Pattern Matching Spec #35. This issue should be a year or two in the future however, so new things may come to make this issue moot (hopefully). We cannot be the only ones that would want that type of functionality.
@Vlek Vlek added the enhancement New feature or request label Feb 21, 2021
@Vlek Vlek self-assigned this Feb 21, 2021
@Vlek
Copy link
Owner Author

Vlek commented Feb 21, 2021

Did some checking, this seems rather doable within the pyparsing framework.

Notable areas to look:
infixNotation allows for setting parse actions
setParseAction allows for functions that handle individual parsed elements

My concern now is that we would still likely need to throw around EvaluationResults objects that contain all of the RollResults for click echoing.

@Vlek
Copy link
Owner Author

Vlek commented Feb 21, 2021

More potential pros:

  • Could help with the Cython conversion issue Convert parser to Cython #29 by helping with typehints.
  • Could also help facilitate pretty error message printing in issue Pretty error messages for input parsing issues #19 since everything will be verified during parsing which could potentially make the error stack more helpful and we can let pyparsing figure out where in our expression there was an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant