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

Relax restriction on "let" expression variable names #5231

Open
anandthakker opened this issue Sep 1, 2017 · 3 comments
Open

Relax restriction on "let" expression variable names #5231

anandthakker opened this issue Sep 1, 2017 · 3 comments
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@anandthakker
Copy link
Contributor

Currently, we only allow a-zA-Z0-9 and _ in variable names. This is unnecessarily restrictive and, of course, especially problematic for users of non-Latin alphabets.

We can either:

  • expand the set of explicitly allowed characters ( @1ec5 @ChrisLoer do you know if there's a reasonable/practical way to do this that would work across many/most scripts? )
  • or instead switch to just disallowing characters that aren't valid in JS identifiers.
@anandthakker anandthakker added the cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) label Sep 1, 2017
@ChrisLoer
Copy link
Contributor

expand the set of explicitly allowed characters ( @1ec5 @ChrisLoer do you know if there's a reasonable/practical way to do this that would work across many/most scripts? )

I don't really have any expertise here, but here's my take:

  • The current character set may not be perceived as that restrictive, and maybe we should just wait for feedback.
  • Switching to a blacklist based on JS identifier rules sounds fine, but that behavior is a bit of a moving target, and it also allows for variable names that will render the same on screen but have different values, which seems a bit undesirable to me.

@1ec5
Copy link
Contributor

1ec5 commented Sep 1, 2017

expand the set of explicitly allowed characters ( do you know if there's a reasonable/practical way to do this that would work across many/most scripts? )

This is the subject of Unicode Standard Annex #31, which corresponds to \P{Pattern_Syntax} in ICU regular expressions (Pattern_Syntax being the Unicode property for syntactic punctuation).

We can get less optimal but decent behavior with the :word: POSIX character class, which is used by ICU regular expressions and equivalent to [\p{Alphabetic}\p{Mark}\p{Decimal_Number}\p{Connector_Punctuation}\u200c\u200d]. In JavaScript, RegExp’s u flag turns on Unicode awareness, but \w is nonetheless constrained to [A-Za-z0-9_]. For GL JS, we may be able to roll something compatible with the POSIX character class in script_detection.js.

Whatever the case, I would recommend leaning on an existing standard rather than attempting to roll our own ad-hoc set of allowed characters.

@1ec5
Copy link
Contributor

1ec5 commented Sep 2, 2017

The current character set may not be perceived as that restrictive, and maybe we should just wait for feedback.

This sounds reasonable. For what it’s worth, NSExpression’s format syntax appears to only support ASCII in identifiers, based on the BNF grammar and a cursory glance in a Swift playground. But the +[NSExpression expressionForVariable:] method is still available for anything more exotic:

playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests

3 participants