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] Adding error to catch index out of bounds errors #3788

Merged
merged 27 commits into from
Jan 2, 2023

Conversation

jpelay
Copy link
Member

@jpelay jpelay commented Dec 5, 2022

Description

Adds an exception to the code generated after transpiling to catch index out of bounds errors.
Things to do:

  • Figure out a way to make the message translatable (gettext works inside hedy.py!)
  • Try to show which line and variable caused the error. (Can show the variable, not the line!)

Fixes #3463

How to test

Since this is added after transpiling, should I add an e2e test?

Checklist
Done? Check if you have it all in place using this list:*

  • Contains one of the PR categories in the name
  • Describes changes in the format above
  • Links to an existing issue or discussion
  • Has a "How to test" section

If you're unsure about any of these, don't hesitate to ask. We're here to help!

@jpelay
Copy link
Member Author

jpelay commented Dec 6, 2022

Changed the approach to add exceptions individually before commands that use lists. I think it's better!

image

@jpelay
Copy link
Member Author

jpelay commented Dec 15, 2022

So this is technically ready, but I have to update 71 tests!

image

@jpelay jpelay marked this pull request as ready for review December 21, 2022 00:35
@jpelay jpelay requested a review from Felienne December 21, 2022 00:35
@jpelay jpelay changed the title [Language] [WIP] Adding error to catch index out of bounds errors [Language] Adding error to catch index out of bounds errors Dec 21, 2022
@Felienne
Copy link
Member

Felienne commented Jan 2, 2023

Ah sorry I missed this in between all Cypress tests, sorry! Will take a look now.

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.

Great improvement @jpelay!!

image

@mergify
Copy link
Contributor

mergify bot commented Jan 2, 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
Copy link
Contributor

mergify bot commented Jan 2, 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 736d659 into hedyorg:main Jan 2, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 2, 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).

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.

[BUG] Unclear message "list index out of range"
2 participants