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

Support non-ASCII identifiers #813

Merged
merged 3 commits into from
Aug 21, 2024
Merged

Support non-ASCII identifiers #813

merged 3 commits into from
Aug 21, 2024

Conversation

lotem
Copy link
Contributor

@lotem lotem commented Aug 21, 2024

Support Unicode identifiers.

Refer to the similar language feature in Rust:
rust-lang/rfcs#2457

@lotem
Copy link
Contributor Author

lotem commented Aug 21, 2024

Hi John-John @udoprog,

The non_ascii_idents feature is now merged to stable Rust. It would be nice to have Unicode identifier names in Rune too.
So far the feature seems to work after a few changes. Please advice if there's more to do.

@udoprog
Copy link
Collaborator

udoprog commented Aug 21, 2024

Hi!

This looks great, thanks for the PR!

Please move the test into crates/rune/tests. They will be picked up automatically there.

A cargo feature flag is not appropriate since it transitively changes the behavior of anyone using Rune if one dependency activates it. In my view we should just adopt this for 0.14. If we do think it's an appropriate optional feature, add a an Options flag so that individual projects can decide themselves whether or not to activate it.

@udoprog udoprog added enhancement New feature or request rust-likeness Issues related to making Rune more like Rust through feature parity. labels Aug 21, 2024
The feature `non_ascii_idents` gives Rune feature parity with Rust in
supporting Unicode identifiers. Refer to:
rust-lang/rfcs#2457
@lotem
Copy link
Contributor Author

lotem commented Aug 21, 2024

Thanks for the suggestions. I'll update the PR later.

@lotem
Copy link
Contributor Author

lotem commented Aug 21, 2024

I was worried about introducing a dependency for a rarely needed feature. Otherwise I don't think users will ever want to disable it since it's a compatible change. Also the performance report from unicode-ident seems to be less of a concern.

Enable non-ASCII identifiers by default.
@udoprog
Copy link
Collaborator

udoprog commented Aug 21, 2024

The dependency is not a problem so please go ahead.

@udoprog udoprog enabled auto-merge (squash) August 21, 2024 15:49
@udoprog
Copy link
Collaborator

udoprog commented Aug 21, 2024

Thank you!

@lotem lotem changed the title Add feature: non-ASCII identifiers Support non-ASCII identifiers Aug 21, 2024
@udoprog udoprog merged commit 812da73 into rune-rs:main Aug 21, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust-likeness Issues related to making Rune more like Rust through feature parity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants