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

🪲 Fix error translation and highlighting #5275

Merged
merged 7 commits into from
Mar 21, 2024
Merged

Conversation

boryanagoncharenko
Copy link
Collaborator

Fixes #5240

  • highlighing parts of an error is now defined in the error template (in *.po files) using backticks.
  • all error templates (in the *.po files) are adjusted to use the new highlighting mechanism.
  • duplicated logic for translation and highlighting of errors existed in 3 places in the code base: app.py for errors during transpiling, hedy.py for transpiled errors and statistics.py. The logic is now extracted and unified in hedy_error.py.
  • we have tests to check that every error can be formatted with the provided arguments for all languages. However, since we started using safeFormat, these tests could not fail anymore even when there were missing keys. The safeFormat is now removed, the failing tests are corrected and a separate test is added to confirm key errors are raised.

How to test

  • Automated unit tests are added to cover the new translation and highlighting mechanism, so all tests must pass.
  • Choose a subset of errors and check that they appear correctly on the website. For example:
    In level 4:
is Foobar
print welcome

In level 6, supply text value to the ask command instead of a number (note that currently the error should referring to the value 'a' which is not nice but will be addressed in #5005)

a = ask 'how much?'
print 360 * a

In level 8:

repeat 5

In level 16:

test = 1, 2, 3

In level 16. Change the language and ensure that the keyword 'random' in the last example is translated correctly:

print list at 12

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Boryana. I always love your work! I tried this, and everything looks fine, but I wanted to ask if there was a way to address #5252 in this PR as well? I think the problem in that case is that allowed_types on the Invalid Argument string in messages.po

@boryanagoncharenko
Copy link
Collaborator Author

Thanks for working on this Boryana. I always love your work! I tried this, and everything looks fine, but I wanted to ask if there was a way to address #5252 in this PR as well? I think the problem in that case is that allowed_types on the Invalid Argument string in messages.po

Thanks for pointing out #5252. It does impact the logic I created indeed. The issue turned out to be that sometimes translated argument values use new keys that need to be translated in turn. As you can imagine, we can go crazy with the nested translations, so I chose to make it work with 1 level of nested translations for now. Maybe if we encounter the need to support deeper nesting (e.g. 2 or 3 levels), we will add the functionality.

However, I am still hesitant about how to deal with which arguments need to be translated (the TODO: in the hedy_errors.py). I do not want to maintain a list of keys as we currently do because it is error prone. The responsibility for knowing what should be translated could lay:

  • in the .po files, for example, a key which requires translation could be noted with double brackets {{}} or something like that
  • in the transpiler. When a HedyException is created it would have the already translated arguments.
  • in the HedyException itself. There could be an extra field containing the list of arguments that need to be translated.

Obviously this question requires a separate issue to be created. However, if anything comes to mind while you are reviewing, please let me know!

@jpelay
Copy link
Member

jpelay commented Mar 21, 2024

Thanks for working on this Boryana. I always love your work! I tried this, and everything looks fine, but I wanted to ask if there was a way to address #5252 in this PR as well? I think the problem in that case is that allowed_types on the Invalid Argument string in messages.po

Thanks for pointing out #5252. It does impact the logic I created indeed. The issue turned out to be that sometimes translated argument values use new keys that need to be translated in turn. As you can imagine, we can go crazy with the nested translations, so I chose to make it work with 1 level of nested translations for now. Maybe if we encounter the need to support deeper nesting (e.g. 2 or 3 levels), we will add the functionality.

However, I am still hesitant about how to deal with which arguments need to be translated (the TODO: in the hedy_errors.py). I do not want to maintain a list of keys as we currently do because it is error prone. The responsibility for knowing what should be translated could lay:

  • in the .po files, for example, a key which requires translation could be noted with double brackets {{}} or something like that
  • in the transpiler. When a HedyException is created it would have the already translated arguments.
  • in the HedyException itself. There could be an extra field containing the list of arguments that need to be translated.

Obviously this question requires a separate issue to be created. However, if anything comes to mind while you are reviewing, please let me know!

Hi! Sorry for not replying sooner. Each one of those options I think poses its own set of problems, but in my opinion I think the third one is a nice middle ground, the first one also makes sense, and I think we haven't had issues with mistranslated fields on Weblate, so that would also be nice!

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Thank you Boryana!

Copy link
Contributor

mergify bot commented Mar 21, 2024

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 fb3acc8 into main Mar 21, 2024
12 checks passed
@mergify mergify bot deleted the error_translation_5240 branch March 21, 2024 16:02
Copy link
Contributor

mergify bot commented Mar 21, 2024

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🪲 ugly error messages
3 participants