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

[LANGUAGE] improved error productions for non decimals #4817

Merged
merged 7 commits into from
Dec 5, 2023

Conversation

akseron
Copy link
Collaborator

@akseron akseron commented Nov 28, 2023

Changes in error production and regex to support better syntax error messages for non decimal numbers. Currently regex allows non decimals, however they are unfunctional and provide complex parse errors. Fixes #4670.

Works for all tests except:

TestsLevel17.test_if_no_colon_after_pressed_gives_parse_error

where

E   AssertionError: False is not true

error is given.
@ghost
Copy link

ghost commented Nov 28, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

Hi @akseron!

Just to check, is this a fix for #4670?

Also, I see it is said to draft, do you plan more work? I can help with the merging and failing tests if you want!

@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

Hi @akseron!

I updated the broken test. I am not sure why the parser gets confused with the new rule, but it is ok for me, the previous location was also wrong haha, and we need to aim to not give a ParseError anyway in such a situation.

So I am good with shipping this, If you are also, set it to Ready for Review and I will approve!

@akseron
Copy link
Collaborator Author

akseron commented Dec 5, 2023

Oh great thank you! I was already planning to ask about changing the error_location since it seemed hard coded but it's great that you already fixed it haha. It was indeed a fix to 4670 so this should do it.

A last question I had about it was that during the last Hedy contributors meeting, I was told I had to refactor the code but I am not certain if that is necessary here. I will set it to Ready to Review in case it is not 👍

@akseron akseron marked this pull request as ready for review December 5, 2023 07:24
@akseron akseron requested a review from Felienne December 5, 2023 07:24
@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

Oh great thank you! I was already planning to ask about changing the error_location since it seemed hard coded but it's great that you already fixed it haha. It was indeed a fix to 4670 so this should do it.

Perfect! Can you add that to the original description? If you add "fixes #4670" then the issue will close automatically!

A last question I had about it was that during the last Hedy contributors meeting, I was told I had to refactor the code but I am not certain if that is necessary here.

Ah I wam not there so I don't know who said that and what refactoring they asked for (just "refactoring" is a but vague) Maybe you can ask who said that for their input?

@jpelay
Copy link
Member

jpelay commented Dec 5, 2023

Hi @akseron! You don't need to refactor your code, that's true! I thought you needed to force push main back, but you're right that that is not necessary!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the hard work @akseron!

Copy link
Contributor

mergify bot commented Dec 5, 2023

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).

@mergify mergify bot merged commit b13d4ff into main Dec 5, 2023
11 checks passed
@mergify mergify bot deleted the error-productions-2 branch December 5, 2023 21:33
Copy link
Contributor

mergify bot commented Dec 5, 2023

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).

@akseron akseron restored the error-productions-2 branch March 3, 2024 23:23
@akseron akseron deleted the error-productions-2 branch May 29, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LANGUAGE] Hedy doesn't fully support non-positional numeral systems
3 participants