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

💻 Add curly braces around teacher adventures' code #5253

Merged
merged 20 commits into from
Mar 19, 2024

Conversation

jpelay
Copy link
Member

@jpelay jpelay commented Mar 12, 2024

Adds curly braces automatically around keywords in the teacher's adventure code.

For that, we iterate over the code snippets inside the contents of the editor, and use Lezer to parse and the code, and eventually iterate over the syntax tree to add curly braces around the keywords. The generated code should be as close as possible to the original one, maintaining indentation levels, spaces, and even errors.

Also adds a warning for the case when the teacher selects several levels and also have code snippets, letting them know this can lead to trouble.

Fixes #5140

How to test

  • Automated tests should pass (WIP)
  • Create an adventure, and write your code normally.
  • Log in as a student, or preview the class where that adventure was added and see that you can change the keywords language.
  • You could also see the generated code by going to the local database, searching for the adventure you just created and check that the keywords have curly braces around them.

@jpelay
Copy link
Member Author

jpelay commented Mar 12, 2024

This is the design I have in mind for the warning. Just need to figure out how to hook an event to the select for the levels.

image

@jpelay
Copy link
Member Author

jpelay commented Mar 14, 2024

@Felienne This PR is almost ready but there's something I haven't been able to figure out and would like to hear your input! Adding curly braces around keywords in code snippets is possible due to Lezer, and since the keywords have context attached to them, in the case of keywords using the same word, we can know by context which one is right. However, what to do with keywords that aren't part of snippets, like this:

image

Just iterating over the keyword regexes, and returning the one that matches is not enough, because there might be several of them. How can we handle this?

@hasan-sh
Copy link
Collaborator

Just iterating over the keyword regexes, and returning the one that matches is not enough, because there might be several of them.

Why not? You could iterate over all the code tags (that you get from CKEditor for free) and then surround them by {}, if the teacher didn't do so!

@jpelay
Copy link
Member Author

jpelay commented Mar 14, 2024

Why not? You could iterate over all the code tags (that you get from CKEditor for free) and then surround them by {}, if the teacher didn't do so!

We've already had this problem in the past (see #4898), but essentially the problem is that if the teacher writes: to, for example, then there are 2 keywords that match that word: to_list and to, and both of these might have different translations in different languages. You can see the json we use for this here

@hasan-sh
Copy link
Collaborator

Why not? You could iterate over all the code tags (that you get from CKEditor for free) and then surround them by {}, if the teacher didn't do so!

We've already had this problem in the past (see #4898), but essentially the problem is that if the teacher writes: to, for example, then there are 2 keywords that match that word: to_list and to, and both of these might have different translations in different languages. You can see the json we use for this here

I'm merely trying to think with you! We are only concered with the keys that we add to the keywords and not their translations, i think. So, if they write to, we match it with the key we have no matter what its translation is! And if there's a keyword that doesn't really exist, we just don't surround it with {}!

@jpelay
Copy link
Member Author

jpelay commented Mar 14, 2024

I'm merely trying to think with you!

Sorry if the message sounded dry! Thanks for helping out!

So, if they write to, we match it with the key we have no matter what its translation is!

This is not really possible! The format we use for the translation of keywords is the following:

"keyword_code": "language_regex | english_word"

an example of this is:

        "add": "ـ*اـ*ضـ*فـ*|add",
        "and": "ـ*وـ*|and",
        "ask": "ـ*اـ*سـ*أـ*لـ*|ask",
        "at": "ـ*بـ*شـ*كـ*لـ*|at",
        "black": "ـ*اـ*سـ*وـ*دـ*|black",
        "blue": "ـ*اـ*زـ*رـ*قـ*|blue",
        "brown": "ـ*بـ*نـ*يـ*|brown",

If the user writes ask then, what I'll iterate over each regex in the dictionary, and for the first one that matches, I'll return the keyword code related to that regex, in this case the regex being "ـ*اـ*سـ*أـ*لـ*|ask and the keyword ask. To be able to properly format the keyword in the frontend, we need to put the curlies around the keyword_code.

So, let's see this case, in Spanish we have these two keywords that use the word, but in English the word is different:

"at": "en|at",
"in": "en|in",

So if the teacher writes en and I return at, when the kid hits the language switcher, then the keyword shown will be at and not in.

But maybe I'm being dumb and this is what you're suggesting? In any case, hope I'm not sounding dismissive, thanks a lot for helping out!

@jpelay jpelay marked this pull request as ready for review March 14, 2024 19:25
@jpelay jpelay requested a review from Felienne March 14, 2024 19:25
@jpelay
Copy link
Member Author

jpelay commented Mar 14, 2024

I'm marking this as ready to review, because maybe we can deal with the individual keywords in other PR!

@Felienne
Copy link
Member

I'm marking this as ready to review, because maybe we can deal with the individual keywords in other PR!

ok! Was the confusion above addressed (ie, did Hasan's solution make sense?)

@Felienne
Copy link
Member

Felienne commented Mar 18, 2024

In any case, it works!

image

And I love that if I add curlies, it still works!!

image

@Felienne
Copy link
Member

My only comment would be (and this can also be doen in a separate PR!) that the examples still have curlies, that now are not needed anymore I think?

image

@jpelay
Copy link
Member Author

jpelay commented Mar 18, 2024

ok! Was the confusion above addressed (ie, did Hasan's solution make sense?)

No, not really! That's why I think it might be addressed in a different PR, I have some ideas but maybe we can discuss them tomorrow?

@Felienne
Copy link
Member

ok! Was the confusion above addressed (ie, did Hasan's solution make sense?)

I have some ideas but maybe we can discuss them tomorrow?

Absolutely! Shall we merge this as is (it does work!) or do you want to tak the content suggestion I made above into account here?

@jpelay
Copy link
Member Author

jpelay commented Mar 18, 2024

Absolutely! Shall we merge this as is (it does work!) or do you want to tak the content suggestion I made above into account here?

I removed the curly braces from the code blocks in English, Spanish and Dutch. Would that be enough or do I remove them from every language?

@Felienne
Copy link
Member

Absolutely! Shall we merge this as is (it does work!) or do you want to tak the content suggestion I made above into account here?

I removed the curly braces from the code blocks in English, Spanish and Dutch. Would that be enough or do I remove them from every language?

Ideally, yes! I tried to do a few more. I also thought that the first {print} needed to be removed, see 63fd54d. Is that correct? If not... feel free to revert the commit!

@jpelay
Copy link
Member Author

jpelay commented Mar 18, 2024

Ideally, yes! I tried to do a few more. I also thought that the first {print} needed to be removed, see 63fd54d. Is that correct? If not... feel free to revert the commit!

I did revert the commit because curly braces around those keywords are still needed, I also removed the unnecessary curlies in the languages that were left!

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.

Perfect, thanks for the extra effort on the content!

Copy link
Contributor

mergify bot commented Mar 19, 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 5f191df into main Mar 19, 2024
12 checks passed
@mergify mergify bot deleted the automatically_add_curly_braces branch March 19, 2024 11:25
Copy link
Contributor

mergify bot commented Mar 19, 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💻 add { } for keywords automatically
3 participants