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

Editorial: Clarify RegExp grammar parameter U → UnicodeMode #2411

Merged
merged 1 commit into from
Aug 15, 2021

Conversation

mathiasbynens
Copy link
Member

@mathiasbynens mathiasbynens commented May 20, 2021

This makes the parameter more easily searchable while clarifying its meaning.

This came up while working on the draft spec for the RegExp set notation proposal, which introduces another RegExp grammar parameter that is related to Unicode. Rather than sticking to single letters, it seems nicer to move to more descriptive names.

If the editors are open to this change, I’d be happy to work on a patch that renames N as well (perhaps to WithNamedCaptureGroups).

cc @markusicu @markusicu @sffc

@mathiasbynens mathiasbynens force-pushed the rename-regexp-grammar-parameters branch from 0739d67 to e0cea50 Compare May 20, 2021 17:01
@markusicu
Copy link
Contributor

Bike-shedding opinion:

  • Longer grammar parameters would be more searchable (as you say) and more guessable, e.g., U vs. the Unicode boolean in the semantics
  • We could use the same terms for grammar parameters and semantics booleans (e.g., Unicode), but it might also be confusing.
  • For the syntax grammar, very long parameter names could be tedious to read & write, especially where multiple parameters are used on a line.
  • Maybe intermediate abbreviations for the grammar parameters? E.g., UUni, (proposed) VUniSet, NNamed

@mathiasbynens
Copy link
Member Author

  • Longer grammar parameters would be more searchable (as you say) and more guessable, e.g., U vs. the Unicode boolean in the semantics

+1

  • We could use the same terms for grammar parameters and semantics booleans (e.g., Unicode), but it might also be confusing.

I think it would mostly be confusing, as it would imply a connection that isn't there.

  • For the syntax grammar, very long parameter names could be tedious to read & write, especially where multiple parameters are used on a line.
  • Maybe intermediate abbreviations for the grammar parameters? E.g., UUni, (proposed) VUniSet, NNamed

That reduces searchability. Ctrl+F "Uni" would return lots of unrelated hits.

jmdyck
jmdyck previously requested changes May 25, 2021
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb requested a review from jmdyck May 25, 2021 18:05
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks fine now.

@ljharb ljharb requested review from syg, michaelficarra, bakkot and a team May 25, 2021 20:46
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label May 25, 2021
@michaelficarra
Copy link
Member

I'd rather see us have a consistent editorial direction here, either going the verbose route for all grammar flags or the terse route for all grammar flags. We can discuss the tradeoffs of each in the editor call, but right now I don't think we should do this as a 1-off change.

@mathiasbynens
Copy link
Member Author

mathiasbynens commented May 26, 2021

We can discuss the tradeoffs of each in the editor call, but right now I don't think we should do this as a 1-off change.

Just to be clear, I agree with this! As I said:

If the editors are open to this change, I’d be happy to work on a patch that renames N as well (perhaps to WithNamedCaptureGroups).

The reason I proposed doing it as two separate patches is because this one would (editorially) affect the RegExp set notation spec proposal whereas the N change wouldn’t. But I’d be happy to update this patch to change both at once if that's preferred.

@michaelficarra
Copy link
Member

We talked about this in the editor call. Longer term, we would like to link all of the grammar flags to a place that has descriptions. But for now, changing U to something more descriptive is fine. We just ask that flag names not include Flag in the name (even though we understand that here it is referring to the RegExp flags).

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jun 4, 2021
@mathiasbynens
Copy link
Member Author

mathiasbynens commented Jun 10, 2021

We talked about this in the editor call. Longer term, we would like to link all of the grammar flags to a place that has descriptions. But for now, changing U to something more descriptive is fine. We just ask that flag names not include Flag in the name (even though we understand that here it is referring to the RegExp flags).

Any suggested new name for UnicodeFlag? Perhaps UnicodeMode or WithUnicode (to match the potential WithNamedCaptureGroups in the future)? It would help to know the exact rationale for avoiding "Flag" (because maybe "Mode" is a no-go for the same reason). For context, I'm trying to think of a name that's unique within the spec document, to improve searchability — so I’d rather not go with just Unicode.

@michaelficarra
Copy link
Member

UnicodeMode sounds good to me. Two reasons not to use "flag" in the name:

  1. A reader is probably more likely to think the word refers to it being a grammar flag than a RegExp flag, and we certainly aren't going to add Flag to the end of every grammar flag.
  2. The grammar flags should be referring to the mode that the grammar is in or the thing that is being controlled, not the cause for entering this mode. For example, the Await flag is called Await because it controls the handling of the await contextual keyword. It is not called Async even though it is introduced by the async keyword.

@mathiasbynens mathiasbynens force-pushed the rename-regexp-grammar-parameters branch from 3c626bf to b025795 Compare June 10, 2021 17:13
@mathiasbynens
Copy link
Member Author

I’ve gone ahead and updated the patch to use UnicodeMode. PTAL

  • A reader is probably more likely to think the word refers to it being a grammar flag than a RegExp flag, and we certainly aren't going to add Flag to the end of every grammar flag.

That's interesting. I never thought of these as "grammar flags". The spec refers to them as grammar parameters.

@markusicu
Copy link
Contributor

I suggest changing the PR title to say "UnicodeMode"

@mathiasbynens mathiasbynens changed the title Editorial: Clarify RegExp grammar parameter U → UnicodeFlag Editorial: Clarify RegExp grammar parameter U → UnicodeMode Jun 11, 2021
@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@mathiasbynens
Copy link
Member Author

Is anything blocking this? I'd be happy to resolve the conflicts if that moves things along.

@michaelficarra
Copy link
Member

@mathiasbynens That won't be necessary. We just need @bakkot or @syg to find some time to do a review.

@bakkot bakkot force-pushed the rename-regexp-grammar-parameters branch from b025795 to d03e65a Compare August 15, 2021 18:11
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

This LGTM, sorry for the delay. I've also rebased, so it should be good to land.

@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. editorial change labels Aug 15, 2021
@ljharb ljharb removed the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 15, 2021
@ljharb
Copy link
Member

ljharb commented Aug 15, 2021

@bakkot it looks like the rebase ended up with the wrong commit - d03e65a instead of a rebased b025795.

@bakkot bakkot force-pushed the rename-regexp-grammar-parameters branch from d03e65a to 8d2f270 Compare August 15, 2021 19:57
@bakkot
Copy link
Contributor

bakkot commented Aug 15, 2021

Whoops, fixed.

This makes the parameter more easily searchable while clarifying its meaning.
@ljharb ljharb force-pushed the rename-regexp-grammar-parameters branch from 8d2f270 to 0e25cc2 Compare August 15, 2021 20:53
@ljharb ljharb added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Aug 15, 2021
@ljharb ljharb merged commit 0e25cc2 into tc39:master Aug 15, 2021
@mathiasbynens mathiasbynens deleted the rename-regexp-grammar-parameters branch August 16, 2021 06:29
mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
This makes the parameter more easily searchable while clarifying its meaning.
mathiasbynens added a commit to mathiasbynens/html that referenced this pull request May 9, 2022
domenic pushed a commit to whatwg/html that referenced this pull request May 10, 2022
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Feb 16, 2023
Fixes bug from PR tc39#2436.

(The renaming happened in PR tc39#2411.)
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Feb 17, 2023
Fixes bug from PR tc39#2436.

(The renaming happened in PR tc39#2411.)
mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Jul 12, 2023
This makes the parameter more easily searchable while clarifying its meaning.

Issue: tc39#2411
mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Jul 12, 2023
This makes the parameter more easily searchable while clarifying its meaning.

Issue: tc39#2411
mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Aug 2, 2023
This makes the parameter more easily searchable while clarifying its meaning.

Issue: tc39#2411
ljharb pushed a commit to mathiasbynens/ecma262 that referenced this pull request Aug 16, 2023
…c39#3120)

This makes the parameter more easily searchable while clarifying its meaning.

Issue: tc39#2411
zhangenming pushed a commit to zhangenming/ecma262 that referenced this pull request Dec 22, 2023
…c39#3120)

This makes the parameter more easily searchable while clarifying its meaning.

Issue: tc39#2411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants