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] Adds music to the language #4903

Merged
merged 41 commits into from
Jan 3, 2024
Merged

[LANGUAGE] Adds music to the language #4903

merged 41 commits into from
Jan 3, 2024

Conversation

Felienne
Copy link
Member

@Felienne Felienne commented Dec 13, 2023

How to test

Paste this code in level 1 and be amazed!! 🤣

play C4
play D4
play E4
play C4
play C4
play D4
play E4

TODO/Notes to self:

  • Syntax highlighting
  • Outline a few adventures for the content developers to work on
  • Should we also support integer values, like this: play 21 so that we can do nice calculations with sounds?
  • Should we also support (at a certain level) passing in duration: play C4 0.1 to make nicer music? Maybe in level 6 dealing with numbers of 12 dealing with floats (a bit late... I fear)
  • Should we support playing "chords" aka multiple notes at once? play C4 E4 F4 (note.js supports this easily, just drop the sleep from the Python) That would also require changes to level 3, then we should support multiple arguments with a list_access (like print) not just one
  • From level 12, assignments needs quotation (see the test we add in this commit: n = 'C4' I am not sure I like that, it feels like a note should be a literal in their own right. We can change the parser to support both?)
  • What are note names in other languages? Arabic and Chinese come to mind, but maybe even other Latin languages use different conventions (I have a vague memory of Flemmish Dutch using Do-Re-Me rather than C-D-E?!) This is an argument in favor of (localized) integers 😅) -> I have asked @alaaeddin-swidan who is looking into this for Arabic!
  • Fix parser error for var named player (fixed poorly in fccbef8)
  • Stop the "too long" message for song programs:
image

@ghost
Copy link

ghost commented Dec 13, 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 Felienne changed the title [LANGUAGE] Experimental branch to add MUSIC to Hedy!! [LANGUAGE] Adds music to the language Dec 27, 2023
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.

Hi Felienne! Bellow I explain a bit about how you can make the highlighting work!

@@ -25,6 +25,7 @@
"def",
"return",
"print",
"play",
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to edit these files anymore, since they are related to Ace, and after #4924 is merged, we wouldn't need to keep mainting this files.

@@ -48,6 +48,15 @@
"next": "value",
"unicode": true
},
{
"regex": "(^ *)(__play__)",
Copy link
Member

Choose a reason for hiding this comment

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

Same here!

@@ -1,8 +1,9 @@
@top Program { eol* (Command eol+)* Command? }
Command {
Print | Ask | Echo | Turtle | ErrorInvalid
Print | Ask | Echo | Play | Turtle | ErrorInvalid
Copy link
Member

Choose a reason for hiding this comment

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

This is great! Now, we only need to edit tokens.ts. Since we can change the keywords at run time, we can't define the keywords directly in the grammar, therefore the need for a external specializer.

Now after compiling this parser, go to tokens.ts and modify the import for level 1 too add the play keyword. After this, go to the keywordToToken dictionary and add the keyword in the extend field. And at last, go to cm-editor.ts and edit the hedyStyleTags var to add the new keyword.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Success!!

image

I found all of it except for the hedyStyleTags, so thanks a lot for that pointer!!

@Felienne Felienne added the Weblate-lock Used to mark pull requests that might conflict with Weblate and must be merged in calm Weblate times label Jan 2, 2024
@Felienne Felienne marked this pull request as ready for review January 2, 2024 16:36
@Felienne
Copy link
Member Author

Felienne commented Jan 2, 2024

Hi @jpelay!

I think we should merge this now I have fixed all the tests because it is already so massive! If you are ok with that (and with the changes, esp to the highlighter), can you approve this?

I will make separate issues for the remaining points.

@jpelay
Copy link
Member

jpelay commented Jan 3, 2024

What are note names in other languages? Arabic and Chinese come to mind, but maybe even other Latin languages use different conventions (I have a vague memory of Flemmish Dutch using Do-Re-Me rather than C-D-E?!) This is an argument in favor of (localized) integers 😅)

In Spanish we also use Do-Re-Mi (mi rather than me)! I remember being taught that in elementary school haha

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.

I LOVE this so muuuch!!!!! Also love the inclusion of numbers! Made a simple for with a print a play and it played the notes one by and one, and I thought that was pretty cool. Let's approve this and then tackle the other points in the bullet list

Copy link
Contributor

mergify bot commented Jan 3, 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 3261190 into main Jan 3, 2024
2 of 3 checks passed
@mergify mergify bot deleted the music branch January 3, 2024 19:02
Copy link
Contributor

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

@Felienne Felienne added the music label Jan 5, 2024
@Felienne Felienne self-assigned this Oct 18, 2024
Copy link
Contributor

mergify bot commented Oct 18, 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
music Weblate-lock Used to mark pull requests that might conflict with Weblate and must be merged in calm Weblate times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants