tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Clarify "private name" vs "private name identifier" per issue #185 #186

Closed tjcrowder closed 5 years ago

littledan commented 5 years ago

Otherwise, this PR LGTM

tjcrowder commented 5 years ago

@littledan -

On the one hand, I really like it, it's less awkward. On the other hand, "PrivateNameIdentifier" does say exactly what it is (an identifier for a Private Name), which helps keep it related.

For instance, making this change, I ran into the Lexically scoped Private Names section title, which in the existing PR opens like this:

Lexically scoped Private Names

Private name identifiers follow typical lexical scoping logic. This section contains amended algorithms for managing the scope of private name identifiers.

with the change, it becomes

Lexically scoped Private Names

Private identifiers follow typical lexical scoping logic. This section contains amended algorithms for managing the scope of private identifiers.

which seems slightly disconnected, so if making this change, I'd lean toward changing the section title as well:

Lexically Scoped Private Identifiers

Private identifiers follow typical lexical scoping logic. This section contains amended algorithms for managing the scope of private identifiers.

(That might be better anyway, though.)

I also wonder if PrivateNameEnvironment would want to change to PrivateIdentifierEnvironment, since its purpose is to map private identifiers to Private Names... That's more fuzzy.

I have most of the changes made (to see how it worked, not like it was hard). Let me know what you'd like:

  1. Don't change it, stick with PrivateNameIdentifer.
  2. Just change PrivateNameIdentifer to PrivateIdentifier and change private name identifier to private identifier in the text.
  3. Also change the section title above to "Lexically Scoped Private Identifiers".
  4. Also change PrivateNameEnvironment to PrivateIdentifierEnvironment.
littledan commented 5 years ago

I am leaning to 2+3. Even if "private name identifier" is more descriptive, it is so long that it ends up reading a bit like word salad in a larger text. Let's let this sit for a couple days to see if anyone else has input.

tjcrowder commented 5 years ago

@littledan - Here's a question: Given how private methods modifies the use of Private Name, giving it a [[Descriptor]] with fields like [[Type]], I wonder if it's really a "name" anymore? Maybe the terminology should be

?

Don't want to second-guess, and don't want to waste a lot of your time on naming. Just the more I got hands-on with the proposals, the less like a "name" it felt.

Whatever direction you decide to go, please don't hesitate to offload the legwork for it (or other similar things) onto me if it's useful to you.

littledan commented 5 years ago

Yeah, I like this phrasing, private identifier vs private record. Glad you're thinking this through carefully.

littledan commented 5 years ago

Ping, are you planning on doing this private identifier update?

tjcrowder commented 5 years ago

@littledan - Yup, happy to do it if we're ready? Changing PrivateName to PrivateRecord is a big change (not in terms of text, but terminology), wasn't sure whether more discussion was needed. (Sorry, I don't have a feel for that yet. I'll get there.) If there's no reason to delay, I'll do it in the morning.

tjcrowder commented 5 years ago

@littledan - (Replacing earlier comment) Nearly done, but late start this morning, will have to finish it in the morning (out of the office remainder of the day). Two questions:

1. I wonder if we need to include §8.3 - Execution Contexts so we can add PrivateEnvironment to Table 22 and other bits of the text? It's a big section (is it possible to include only the parts of it that need additions?). The part we need is there, I just managed to miss it.)

2. Any objection if I add

"scripts": {
  "build": "ecmarkup spec.html rendered.html"
},

to package.json and a .gitignore ignoring rendered.html? (For development. I assume there's some commit hook or similar on github that updates https://tc39.github.io/proposal-class-fields/.)