Ensure equal constructed and parsed keyword objects #94
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
The grammar, the spec, and the field's type annotation all say that the field's value should be a province without its coast, but the object constructed through parsing had a `Location` in the field instead. I have fixed `DAIDEVisitor` to construct the object properly, added code to fix an incorrectly passed-in type (like some other objects do), and added tests to ensure there are no regressions.
Hashability tests were added to all press keywords, but I wanted to make sure base keywords were also tested the same way.
It was likely left in by mistake.
This based on code I wrote for the ALLAN bot.
I fixed `SUP` earlier, but I figured I should ensure all objects could be constructed properly. I fixed cases where they could not be.
The same case is tested in the `test_PCE_bad()` function.
The original intention was clear from neighboring functions.
|
@aphedges I'll review it now. |
mjspeck
approved these changes
May 31, 2023
|
@aphedges do you have a preference on how we merge this? |
|
@mjspeck, thanks for the review! I would prefer you merge it with a merge commit so you can keep the history. |
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.
I while working on the ALLAN bot, I encountered a bug where
SUP.province_no_coastwas being parsed to the wrong type. I fixed it, but I figured I should check for similar cases. It turns out that although we had full coverage of the formassert str(daide_obj) == daide_str, we had no checks for the formassert daide_object == parse_daide(daide_str). I have added these checks, fixed the cases where they did not passed, and made some small changes for other problems I noticed during the process.