search

tc39 / proposal-regexp-named-groups

Named capture groups for JavaScript RegExps
https://tc39.github.io/proposal-regexp-named-groups/
222 stars 21 forks source link

Review nits #35

Open anba opened 7 years ago

anba commented 7 years ago

https://tc39.github.io/proposal-regexp-named-groups/#sec-backreference-matcher

Let len be s's length.

should be:

Let len be the number of elements in s.

because s is a List and the length of a List is typically accessed through the term "the number of elements in ...".

https://tc39.github.io/proposal-regexp-named-groups/#sec-atomescape

https://tc39.github.io/proposal-regexp-named-groups/#sec-regexpbuiltinexec https://tc39.github.io/proposal-regexp-named-groups/#sec-getsubstitution https://tc39.github.io/proposal-regexp-named-groups/#sec-regexp.prototype-@@replace

https://tc39.github.io/proposal-regexp-named-groups/#annex-b-grammar

mathiasbynens commented 7 years ago

I noticed a few (but not all) of these during a review today and fixed them locally while offline. I’ll submit a PR once I get back to my laptop. The remaining nits should be easy to fix, too. Thanks!

On Nov 2, 2017 17:09, "André Bargull" notifications@github.com wrote:

https://tc39.github.io/proposal-regexp-named-groups/# sec-backreference-matcher

Let len be s's length.

should be:

Let len be the number of elements in s.

because s is a List and the length of a List is typically accessed through the term "the number of elements in ...".

https://tc39.github.io/proposal-regexp-named-groups/#sec-atomescape

  • Typo: "an RegExpIdentifierName" -> s/an/a/
  • The "for an RegExpIdentifierName" in "Search the enclosing RegExp for an instance of a GroupSpecifier for an RegExpIdentifierName which has ..." doesn't sound grammatically correct to me.

https://tc39.github.io/proposal-regexp-named-groups/#sec-regexpbuiltinexec https://tc39.github.io/proposal-regexp-named-groups/#sec-getsubstitution https://tc39.github.io/proposal-regexp-named-groups/# sec-regexp.prototype-@@replace

  • If-steps with sub-steps need a trailing, ", then".

https://tc39.github.io/proposal-regexp-named-groups/#annex-b-grammar

  • AtomEscape :: [+N] k GroupName is missing
  • I don't understand why SourceCharacterIdentityEscape[N] :: [+N] SourceCharacter but not one of c or k was added. Annex B only has IdentityEscape :: [~U] SourceCharacter but not c to ensure /\c/ is parsed as /\c/ instead of /c/, but I don't see why that monstrosity is also needed for k. Am I overlooking some detail?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tc39/proposal-regexp-named-groups/issues/35, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFAFnTHE9L1AtBPvOj9hQU2czQH4Vjlks5syj2YgaJpZM4QQagd .

mathiasbynens commented 7 years ago

My PR is at #36. Note that it’s not a strict subset of what @anba found. (E.g., isn‘t ClassAtomNoDash[U] missing too?)

anba commented 7 years ago

I don't understand why SourceCharacterIdentityEscape[N] :: [+N] SourceCharacter but not one of c or k was added. Annex B only has IdentityEscape :: [~U] SourceCharacter but not c to ensure /\c/ is parsed as /\c/ instead of /c/, but I don't see why that monstrosity is also needed for k. Am I overlooking some detail?

Ah never mind, I found out why this is needed. It's to ensure /\k(?<a>)/ is a SyntaxError.

anba commented 6 years ago

Not all issues mentioned in https://github.com/tc39/proposal-regexp-named-groups/issues/35#issue- 270823718 have been resolved in #38. Maybe reopen?

mathiasbynens commented 6 years ago

Which issues have not been resolved?

anba commented 6 years ago

Which issues have not been resolved?

https://tc39.github.io/proposal-regexp-named-groups/#sec-backreference-matcher

Let len be s's length.

should be:

Let len be the number of elements in s.

because s is a List and the length of a List is typically accessed through the term "the number of elements in ...".

https://tc39.github.io/proposal-regexp-named-groups/#sec-atomescape

Typo: "an RegExpIdentifierName" -> s/an/a/
The "for an RegExpIdentifierName" in "Search the enclosing RegExp for an instance of a GroupSpecifier for an RegExpIdentifierName which has ..." doesn't sound grammatically correct to me.
mathiasbynens commented 6 years ago
littledan commented 6 years ago

Sorry about this! I updated gh-pages.

anba commented 6 years ago

I included those changes in 6387879, having noticed some of them myself. It’s just that the gh-pages branch hasn’t been updated since that got merged.

Err, nope. :-)

The three issues mentioned above are still present in current master.

https://github.com/tc39/proposal-regexp-named-groups/blob/b7940ce153554e7f44d79d97e9536524b91111f3/spec.html#L114 https://github.com/tc39/proposal-regexp-named-groups/blob/b7940ce153554e7f44d79d97e9536524b91111f3/spec.html#L129

littledan commented 6 years ago

@anba Sorry about that; should be better now.

mathiasbynens commented 6 years ago

Err, nope. :-)

Whoops, I might’ve been thinking about the upstream spec patch, which has your corrections: https://github.com/tc39/ecma262/pull/1027/files Sorry I forgot to “backport” these :)

anba commented 6 years ago

@mathiasbynens Ah, I see. So confusing having to track the same proposal in two places. :-)