Enhanced verbose printing #55
Merged
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
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.
Keep notation
While working on coverage, some issues were resolved: 1) Because of how the DiceParser object was being instantiated and used across multiple tests, the RollOption.Maximize was being used in more cases than it was supposed to. Once this was resolved, other tests showed that they did not have correct ranges. The output was reviewed and either the test was updated or the code was to accomodate expected results. 2) Some operations were not being used and it became apparent by the coverage report. This was also the case for types that did not need to have their logic present in things like __rmod__ where the __mod__ method would handle the case instead. 3) There are a few methods for objects that we will want to keep because they will be needed later on that are not current used. To make sure that our coverage is complete, I had to test these internal files instead of end-to-end testing. After we add the new features, we should re-evaluate these additions to consider whether they're actually needed. To do this, we should run pytest specifically against test_roll_dice.py or other scripts that do end-to-end testing only.
Had to increment the python version due to the type notations that we are using. The annotations are not available in 3.6 unfortunately.
Ensured 100% test coverage.
Finding out that we can make things more sane, like removing the click-based printing logic to the EvaluationResults object. I think this is going to make the code much more streamlined as well as add the functionality.
There was an issue with some error messages in our tests, fixed those. Trying to get my head around what it is I should be doing for verbose printing right now. I think the quickest approach is to just keep a history list. This gets dicey though I believe when it's the case that we have multiple EvaluationResults objects where they have their own separate histories. Keeping a global history may be preferable but it doesn't seem right. I'll continue playing with it to see what will give the most accurate response.
I have a working skeleton for what I need to do, now I need to make the modifications to the other operations. Because this is going to require a change in code across the different ops, I think the best thing to do is create a wrapper function to gather the roll info as well as the history at this point.
The new verbose output is going to include all of the info for what has transpired to get us to the point where we have a value. This means that checking whether the string'd output is the value will no longer work. This test is removed and I will now need to make new tests that will deal with the new verbose output. This will also allow for order of operations testing which should be really helpful going forward when adding more to our parser. This really should have been done first. I don't know why I didn't write tests first to ensure I knew what it was that I wanted to make before changing any of the code. Lesson learned.
The code appears to be done but further testing is required to ensure that it is working properly. I should add in the tests using the decorator so that I do not have to keep writing functions for it. We need individual tests for the operations as well as combinations. One thing that's worrying me however is that there appears to be a problem with the order of operations for the roll "2+3-4*5/2". In this case, it's doing the "2+3" addition first, then multiplying, dividing, and finally subtracting. The expected order is for it to multiply, divide, add, and then subtract. This isn't causing any order of ops issues currently, and we have considerable testing to ensure that things are being handled properly currently, but it does cause the history to be out of order. I am not sure if this is simply a history issue or if it's a problem with how the parser is handling things. I first thought that maybe this had to do with the fact that the minus symbol is pulling double duty as it could be being first parsed as a negative instead of as an operation, but I have tried it with addition and substraction and found the same issue. I do not know if this needs to be resolved right away, but it should be investigated at some point. If I do not get it done this time around I will need to make a ticket to track it.
This concludes the verbose printing functionality additions. I have also made some minor fixes. I am very happy with this addition as it was able to increase readability and also help ensure our testing is top notch. The issue with the order of operations with things like "3+2-1*7" still persists. It's wanting to perform the addition first it seems and it will take additional time to sort through. This is a parser issue however and we're already considering changing to a new one anyway.
Codecov Report
@@ Coverage Diff @@
## main #55 +/- ##
===========================================
+ Coverage 99.26% 100.00% +0.73%
===========================================
Files 4 10 +6
Lines 136 367 +231
===========================================
+ Hits 135 367 +232
+ Misses 1 0 -1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
Closes #42