Skip to content

Conversation

rkirsling
Copy link
Member

@rkirsling rkirsling commented Sep 10, 2018

Follow-up to #1239.

On IRC, two ideas were raised as to a consistent, enforceable approach to quoting properties:

  • @ljharb suggested always using quotes unless that property can also be considered an identifier (hence ""prototype" property" but "Function property of the global object" ).

  • @jmdyck suggested using quotes just when referring to it as a property (hence ""Function" property" but "Function constructor", ""toString" property" but "toString method").

Either approach is acceptable to me, but I am starting with the latter, as the diff is more conservative.

I've also included a few related commits which fix neighboring typos, add missing backticks (e.g. to Array.prototype in plaintext), and turn "value foo" into "value foo" where trivial. The last of these could be a separate PR if desired; I've included it here since it too was brought up in the last PR.

@mathiasbynens
Copy link
Member

using quotes for any property that cannot also be considered an identifier (hence ""prototype" property" but "Function property of the global object" )

I don't understand this part. Function can be considered an identifier.

(slightly off-topic because this is not the approach you went with)

@ljharb
Copy link
Member

ljharb commented Sep 10, 2018

Function is an identifier, and a property of the global object, which is why it's a tricky one - so i think it can be argued either way, unfortunately.

@ljharb ljharb requested review from bmeck, bterlson, ljharb and a team September 10, 2018 06:53
@rkirsling
Copy link
Member Author

rkirsling commented Sep 10, 2018

@mathiasbynens Sorry for the unclear wording—what I mean is that because Function can be viewed as an identifier or as a property of the global object (unlike prototype which is not itself an identifier), approach 1 would be to write Function in all cases, while approach 2 would be to treat the case of ""Function" property" as special (while both approaches would write "prototype" in all cases).

We could do either, but the reason approach 1 would be a larger diff is because (I believe) it implies quoting method names everywhere, even in a phrase like "a generic toLocaleString implementation".

@mathiasbynens
Copy link
Member

unlike prototype which is not itself an identifier

prototype is an identifier.

@domenic
Copy link
Member

domenic commented Sep 10, 2018

Everything changed in this PR is an identifier, in fact.

@rkirsling
Copy link
Member Author

@mathiasbynens @domenic Hmm, do you think "bound identifier" would be adequately clear then?

It would seem that given these...

  • 12.1:
    Identifier :
        IdentifierName but not ReservedWord
    
  • 12.3:
    MemberExpression[Yield, Await] :
        ...
        MemberExpression[?Yield, ?Await] . IdentifierName
        ...
    

...and the fact that foo.function = 3; is indeed legal, that to say "a property is not an identifier" shouldn't be incorrect so much as confusable (particularly if the specification of IdentifierName delegates to a Unicode annex that simply talks of "identifiers")? 🤔

But either way, it would be nicer to avoid confusing people, so please let me know. 😄

@domenic
Copy link
Member

domenic commented Sep 11, 2018

I'm not sure what would be clearer, as I'm not sure what distinction you're trying to capture. I gather you think some things should be quoted, and some shouldn't. But so far the justification given is that identifiers should be unquoted, and that doesn't help, because that means everything in this PR should be unquoted. So I'm not sure what you're trying to accomplish.

Separately it seems you are discussing the distinction between identifiers and identifier names, but I'm not sure I see the relevance.

@rkirsling
Copy link
Member Author

I mean, as Mathias noted, this discussion is rather tangential to the current state of the PR. Insofar as it's just me seeking knowledge or opinions, it could be taken to IRC instead.

I imagine the context of this PR is a bit unclear. Admittedly my original idea was rather the opposite (to never quote property names except when actually passing a string literal), but my goal here is nothing other to find a consistent solution that pleases our editors and some hypothetical future linting tool alike.

Thus if we're going to quote property names in general, the key questions are what to do about properties of the global object like Function and what to do about methods like toString.

@bmeck
Copy link
Member

bmeck commented Sep 11, 2018

I'd agree with @domenic that all the changes here seem to be around already valid identifier tokens. I'm not entirely sure if this increases consistency in any way. If it could be explained why these particular tokens are being distinguished and for what reason. Primordials like the Function constructor generally are referred to via primordial syntax like %Function%. Is that the distinguishing feature between these 2 categories is?

@jmdyck
Copy link
Collaborator

jmdyck commented Sep 11, 2018

I'm not entirely sure if this increases consistency in any way.

It does. E.g., in the current spec, there are 74 occurrences of

`"foo"` property

and 133 occurrences of

`foo` property

In rkirsling's quote-all-properties branch, the respective counts are 185 and 23. (The unquoted remnant is mostly due to properties of the global object, as rkirsling raised above.)

It would certainly be possible to resolve the inconsistency in the other direction, it's just that this direction is roughly what came out of the discussion for #1239.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

this change seems to work towards consistency

@rkirsling
Copy link
Member Author

Turns out a chunk of the "unquoted remnant" that @jmdyck mentioned was missed spots (must've been juggling too many regexes, as I have every recollection of addressing them... 🤔), but indeed, the other ones are of two varieties:

  • 'super property', which of course does not mean 'a property named "super"'
  • stuff like 'the Number.parseInt data property', which would need to be rewritten without the dot
    (easily done but probably not necessary, unless the editors would prefer it)

@bterlson also suggested going ahead with converting these "foo" cases into "foo" right away, due to viewing these as ES language values—I'm happy to do so if others are not opposed to it.

@ljharb ljharb requested review from zenparsing and a team January 26, 2019 07:55
@ljharb ljharb removed the request for review from bterlson February 28, 2019 19:57
@rkirsling rkirsling force-pushed the quote-all-properties branch from edeec40 to 0a7a312 Compare May 19, 2019 20:30
@rkirsling
Copy link
Member Author

Rebased, rechecked, and resquashed.

@rkirsling
Copy link
Member Author

Any chance our current editors could review this?

(Just to reiterate, my singular goal here has been to address existing inconsistencies; if we'd like to split this up or reconsider the approach or what have you, that's all fine by me.)

@rkirsling rkirsling force-pushed the quote-all-properties branch from c5702ec to f782f71 Compare June 3, 2019 10:50
@rkirsling
Copy link
Member Author

Resolved conflicts again.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

overall this is a great improvement.

it would be really nice if we had some kind of programmatic enforcement of the formatting, to prevent inevitable future drift.

@ljharb ljharb requested a review from a team June 3, 2019 22:16
@jmdyck
Copy link
Collaborator

jmdyck commented Jun 4, 2019

it would be really nice if we had some kind of programmatic enforcement of the formatting, to prevent inevitable future drift.

A start would be to check that you don't get any output from:

< spec.html grep -o '`[^`".]*` property' | grep -v '`super` property'

and

< spec.html grep -o 'the String `[^`"]*`'

@rkirsling: Running the first of those commands on the current branch finds

`name` property

in Function.prototype.toString, which maybe snuck in on a rebase.

@rkirsling
Copy link
Member Author

(Resolved conflicts.)

@rkirsling rkirsling force-pushed the quote-all-properties branch from ee97ddc to 2d40e7d Compare July 5, 2019 21:54
@rkirsling
Copy link
Member Author

Rebased again.

@rkirsling rkirsling force-pushed the quote-all-properties branch from 2d40e7d to 18e5ffb Compare July 21, 2019 03:35
@rkirsling
Copy link
Member Author

Rebased following #1592.

On that note, if jettisoning the last commit here would make the PR as a whole easier to swallow, that would be fine—it is, after all, barely even the tip of a massive iceberg in itself.

@rkirsling rkirsling force-pushed the quote-all-properties branch from 18e5ffb to deaf4c3 Compare August 11, 2019 00:50
@rkirsling
Copy link
Member Author

Rebased.

@ljharb ljharb force-pushed the quote-all-properties branch from deaf4c3 to d7d8472 Compare October 4, 2019 02:29
ljharb pushed a commit to rkirsling/ecma262 that referenced this pull request Oct 4, 2019
 - Quote properties as `"foo"`.
 - Add missing backticks.
 - value `foo` → value *foo*.
 - Fix a few neighboring typos.
@ljharb ljharb removed request for zenparsing and a team October 4, 2019 02:34
@rkirsling rkirsling force-pushed the quote-all-properties branch from d7d8472 to 1d613b0 Compare October 4, 2019 05:31
 - Quote properties as `"foo"`.
 - Add missing backticks.
 - value `foo` → value *foo*.
 - Fix a few neighboring typos.
@ljharb ljharb force-pushed the quote-all-properties branch from 1d613b0 to fc21887 Compare October 4, 2019 13:49
@ljharb ljharb merged commit fc21887 into tc39:master Oct 4, 2019
@rkirsling rkirsling deleted the quote-all-properties branch October 4, 2019 20:38
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Dec 10, 2019
"NativeError" is a metavariable that stands in for each of the Native Error types,
so it should always be italicized.

The 2 occurrences in headings were italicized in ES5,
but lost their italics in one of the ES6 drafts.

For the value of the 'name' property,
it was italicized when introduced in f3743b5 (PR tc39#270)
but lost its italics in fc21887 (PR tc39#1302).
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Dec 12, 2019
 - Insert missing word "function"
 - add underscores to "M" in a couple spots
 - At "anonymous function", insert "built-in". The usual formula for these clauses is: "A Foo function is an anonymous built-in function ..."
 - Change some "numeric value" to "Number value". They used to be synonyms, but "numeric value" now includes BigInt, which is not meant in these contexts.
 - Italicize "NativeError" in 3 spots. "NativeError" is a metavariable that stands in for each of the Native Error types, so it should always be italicized. The 2 occurrences in headings were italicized in ES5, but lost their italics in one of the ES6 drafts. For the value of the 'name' property, it was italicized when introduced in f3743b5 (tc39#270) but lost its italics in fc21887 (tc39#1302).
 - Refer to parameters in PromiseResolve preamble
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants