Keep notation #46
Merged
Keep notation #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
February 16, 2021 08:21
Nearly done on keep notation, but there are a few edge cases that I at least need to test for and figure out the behavior for. There are things like "(4d6 + 10d4)k4" that would potentially work but I am unsure what it will do and what the correct outcome should be in those cases. I may have it such that the last operation must have been a dice operation in order for the dice modifier operations to work.
Currently working on it and have added the necessary tests.
hole, I was not aware just how much was going to need to change in order to have this option in place. The main issue appears to be that the keep notation requires an implied dice having been rolled beforehand as the last operation. In order to capture this (as well as the rolls that make up the sum), it required additional changes to the flow of the parser that we had in place. There were a few options to attempt to deal with this: 1) Keep most things the same but make it so that, if we have a dice to roll but it includes "k/K int?" at the end, then we would parse it a different way. - This seemed easier but not the best way to handle this. Our parsing lines are already overly verbose and we're getting recursion check errors already by pyparsing so this scared me away from doing it. 2) Create a new object type that would represent the result of an expression that was evaluated during the process of parsing an entire expression. In this way the history of what was parsed and what order could be preserved along with all dice rolls in such a way that we could apply the keep notation to it. This was also going to be compatible with all new dice modifications and would open the door so that we could add them with less difficulty and no recursion warnings/errors. - While I went with this, it's requiring a considerable amount of changes in order to apply. I have written and rewritten changes in order to attempt to add this. I think the main issue I am having right now is that I am testing all of the tests we have but I'm attempting to work using the stable ground principle and it's causing me to have a hard time piecing together what works through all of the errors that we are getting.
there with the operations, only got to do the finishing touches on the dice rolling functions so that they mesh with everything else that we're expecting. I think that moving everything over to EvaluationResults objects is really going to help with some of the things that are coming down the pipeline, as well as making all of the typing easier to handle. All of the veneer functions that are handling the operations are going to greatly simplify the headache attempting to get mypy to agree that everything is syncing correctly. I still have not put in place the method in the parser that is going to call the correct operations. What we're getting from the parser for any order of operations level is a list of a list of ints, floats, EvaluationResults, and strings representing the outcomes, inputs, and operation types that we will be doing. e.g. [[1, "+", 1]]. This is easy enough when considering a single operation, but multiple can be changed like so: [[1, "+", 2, "+", 3]]. In order to handle this, I propose that we solve the first pair first, then use that result with the second given operation and so forth. I believe that we can use a for loop and use the index skip to grab pairs (and be able to say that the value to use with the next operation is op_index + 1)
Merging in the small updates into current feature branch.
together after I separated the project out like I did. I added a few TODO notes in operations because I believe I need to re-look at a few things that I am doing there, including whether we are giving a total sum of the dice rolls with the rolls or whether that can be calculated when needed and whether our floating point logic is right for sides and num_dice.
The keep notation tentatively works. The problem is that this complete rewrite has broken a lot of stuff as well. About 75% of tests are succeeding right now. I have to go back and now figure out what is still not working and fix it.
there are only a handful left, mostly due to the additions made to the inFixNotation scheme that we have for the parser. One issue that I do not know right away how to fix is going to be re-adding the min and max roll options in a sane way to the parser. We can do like we did before and have a global flag that can be used, but I want to see if there is any way we can do it by passing additional info into the pyparsing methods that we're calling. We may have to move away from the inFixNotation that we have as it may not encapsulate what we're doing well. The optional number for (1)d6 for instance is causing a recursion issue how we currently have it stated. It is likely causing the keep notation to not be able to parse strings where the keep dice number is not supplied.
Two major takeaways from this endeavor: 1) When it's the case that our handler functions fail for some reason, they will cause the parser to fail overall and state that there was a parsing issue. This will obfuscate any errors that occur. I was really confused for a good while because of this and had to rely on pytest output where I was catching additional error details to figure this out. In order to solve, will likely have to do the error messages ourselves with the built-in on parse fail function hook. 2) The method that I have chosen to use in order to better handle the insanity of evaluation has caused it to become difficult to then pass whether the dice rolls should be normal, minimized, or maximized. I fell back on the globalized variable option because I knew at least that it would work. There does not appear to be a way within pyparsing to be able to pass additional info when parsing a statement. I am going to open a ticket for consideration of this option because maybe someone else would find this useful as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature inclusion comes with a major refactoring of the codebase to be able to handle what needed to occur to do the feature well.
The keep notation spotlighted a major structural flaw in how things were done previously with the Parse/Evaluate functions being separated from each other. While it is the case that being able to parse without evaluating is helpful for debugging, it was proving to be a nightmare to write a tangled evaluator function that could handle all of the different edge cases that arose out of the number of features that were allowed. Instead, we are now opting to evaluate as we parse, which is greatly reducing the amount of tangle. Each handler function is now able to deal with the input that they're expecting without having to worry about edge cases for other operations that they do not govern.
This unfortunately brought another issue: how do we then pass options from Click to these handler functions when we do not control the parser method from Pyparsing? I fell back on the global variable option, this time having it within the Operations library's global scope. I will revisit when possible, but, without a change in the Pyparser library, I do not believe that there is another good option.
This PR is the final culmination of months of thinking through these issues, trying different solutions, failing, reverting back, and trying again until something clicked and made sense.
Closing #6