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

mini notation: international alphabets support #751

Merged
merged 3 commits into from Oct 26, 2023

Conversation

ilesinge
Copy link
Contributor

Implements #750

Note that without reducing the max error message's length to 180 in the Repl (while keeping the end of the error message), the screen becomes unusable when one use a character that is not included (such as "$"). The full error message can still be found in the console as seen below.
It could be possible to do the same length reduction with the error in the console.

image

@felixroos
Copy link
Collaborator

looks like there are ways to customize the peggy error message: https://peggyjs.org/documentation.html#error-messages maybe it is customizable enough to allow not printing all these characters and just write something like "character x cannot be used"

@ilesinge ilesinge changed the title Support international alphabets in mininotation Support international alphabets in sample names Oct 21, 2023
@ilesinge
Copy link
Contributor Author

Having a look at the error message customization, thanks :)

@felixroos
Copy link
Collaborator

one potential danger i see here is that some of these chars might become useful later to be used as new operators in mini notation. so it will become a breaking change if new characters are needed that are part of this set. many programming languages reserve a bunch of words that might become language features later, even if they are never used in the end.

@ilesinge
Copy link
Contributor Author

It's better with the customized error message, see below.

image

@felixroos
Copy link
Collaborator

It's better with the customized error message, see below.

ah perfect

@ilesinge
Copy link
Contributor Author

Regarding potential operator characters, it should be noted that the added characters do not include special characters, only letters that are not "universally" easily typeable since they are in other alphabets or include diacritics.
I'm not sure they make good candidates for operators.
We could still reduce the list though, but it would be to the detriment of some languages.

@felixroos
Copy link
Collaborator

Regarding potential operator characters, it should be noted that the added characters do not include special characters, only letters that are not "universally" easily typeable since they are in other alphabets or include diacritics. I'm not sure they make good candidates for operators.

good point! so maybe we don't need to reserve any of those then

We could still reduce the list though, but it would be to the detriment of some languages.

i think we can keep it as is then

@felixroos
Copy link
Collaborator

maybe remove the 180 limit again then? More then 180 could be useful in some scenarios as some js errors are really long and might contain useful info at the end

@felixroos felixroos changed the title Support international alphabets in sample names Support international alphabets in mini notation Oct 21, 2023
@felixroos felixroos changed the title Support international alphabets in mini notation mini notation: international alphabet support Oct 21, 2023
@felixroos felixroos changed the title mini notation: international alphabet support mini notation: international alphabets support Oct 21, 2023
@ilesinge
Copy link
Contributor Author

✔️ Removed the 180 limit

@felixroos
Copy link
Collaborator

looks good, I think this is ready to merge then!

@yaxu this is pretty core / potentially tied to tidal compat so I'll ping you to ask if you have any objections?

@yaxu
Copy link
Member

yaxu commented Oct 21, 2023

Fine by me!

@felixroos felixroos merged commit 19f1aad into tidalcycles:main Oct 26, 2023
1 check passed
@ilesinge
Copy link
Contributor Author

Thanks!

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.

None yet

3 participants