[CHORE] move exceptions into isvalid #4838
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
Felienne
commented
Dec 1, 2023
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…to clean-up-invalid
for more information, see https://pre-commit.ci
This reverts commit 310df05.
|
Need to improve the command extraction (maybe rewrite it entirely than trying to improve what we have now!) |
for more information, see https://pre-commit.ci
…to clean-up-invalid
jpelay
approved these changes
Dec 7, 2023
| @@ -63,7 +63,7 @@ def __init__(self, level, line_number, fixed_code, fixed_result): | |||
| level=level, | |||
| line_number=line_number, | |||
| fixed_code=fixed_code, | |||
| fixed_result=fixed_result) | |||
| fixed_result=fixed_result) # what is the difference?? | |||
There was a problem hiding this comment.
Fixed code holds the fixed hedy code, f.e: print hello world. Fixed result holds the ParseResult object of parsing that code
| raise exceptions.MissingCommandException(level=level, line_number=line) | ||
| # IsValid raises the appropriate exception when an error production (starting with error_) | ||
| # is found in the parse tree | ||
| IsValid(level, lang, input_string).transform(program_root) |
There was a problem hiding this comment.
This is a much better way of doing this!
| if level > HEDY_MAX_LEVEL: | ||
| raise Exception(f'Levels over {HEDY_MAX_LEVEL} not implemented yet') | ||
|
|
||
| def create_AST(input_string, level, lang="en"): |
|
Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork). |
Merged
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.
First step towards #4836
This PR cleans up the handling of exceptions a lot! As I explained in #4836 I think I initially was planning to create a function that would return a boolean, is this tree valid or not, with a transformer. But as the code got more advanced, and more experience people worked in it, we started to use exceptions more and it became a bit of a mess.
This trigger for this specific refactoring was there was not an easy way to call one function to detect if a tree was valid, which is needed for the new translation mechanism. So it does not fully close #4836 as I still have to call it on the translation part but this is already quite big and hard to review so let's do that separately.
How to test
Verify nothing breaks :)