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: Consistency for the term "fully populated Property Descriptor" #1241

Merged

Conversation

IgnoredAmbience
Copy link
Contributor

@IgnoredAmbience IgnoredAmbience commented Jun 20, 2018

Previously a mixture of "complete property descriptor" and "fully populated property descriptor" was used. Change to use the defined "fully populated property descriptor" term.

Also capitalise "Property Descriptor" so that definition detection (hopefully) kicks in elsewhere, to be consistent with the rest of the spec.

spec.html Outdated
@@ -1554,7 +1554,7 @@ <h2>[[GetOwnProperty]] (_P_)</h2>
The Type of the return value must be either Property Descriptor or Undefined.
</li>
<li>
If the Type of the return value is Property Descriptor, the return value must be a complete property descriptor (see <emu-xref href="#sec-completepropertydescriptor"></emu-xref>).
If the Type of the return value is Property Descriptor, the return value must be a fully populated Property Descriptor (see <emu-xref href="#sec-completepropertydescriptor"></emu-xref>).
Copy link
Member

Choose a reason for hiding this comment

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

Since the section is named “complete”, should we not go with that for the wording?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to use the term as already defined in the section defining Property Descriptors (sec-property-descriptor-specification-type). Swapping all to be "complete Property Descriptors" is also ok with me, a similar number of changed lines are required.

Copy link
Member

Choose a reason for hiding this comment

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

Fair - https://tc39.github.io/ecma262/#sec-completepropertydescriptor seems to me the stronger precedent. Let’s see what others think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, we may wish to avoid possible confusion between complete (verb) and complete (adjective).

Copy link
Member

Choose a reason for hiding this comment

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

“completed” for the adjective case would work fine.

Copy link
Member

Choose a reason for hiding this comment

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

I think the link to "CompletePropertyDescriptor" in this context is a bit odd; I would expect a definition rather than an algorithm.

I like the change to "fully populated...". Can we just remove the link to the algorithm?

@ljharb ljharb requested review from bterlson, bmeck and allenwb June 20, 2018 16:04
@ljharb ljharb requested review from zenparsing and a team and removed request for allenwb January 26, 2019 07:54
@ljharb ljharb removed the request for review from bterlson February 28, 2019 19:57
@bakkot bakkot force-pushed the editorial-complete-property-descriptor branch from 60107eb to 31f5c30 Compare May 27, 2020 02:50
@bakkot
Copy link
Contributor

bakkot commented May 27, 2020

Rebased. I applied @zenparsing's suggestion to just drop the link to CompletePropertyDescriptor. And since Property Descriptor is a defined term, both of the changed lines now link to "The Property Descriptor Specification Type", which is the section that defines "fully populated property descriptor".

I considered adding a dfn for "fully populated property descriptor", but it didn't seem worth it; the link would go the same section either way, and it wouldn't get linked for "fully populated data Property Descriptor".

@ljharb ljharb requested review from michaelficarra, syg, ljharb and a team May 27, 2020 04:13
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label May 27, 2020
@ljharb ljharb self-assigned this Jun 17, 2020
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jun 17, 2020
ljharb pushed a commit to IgnoredAmbience/ecma262 that referenced this pull request Jun 23, 2020
…tor" (tc39#1241)

Previously a mixture of "complete property descriptor" and "fully
populated property descriptor" was used.

Also capitalise "Property Descriptor" so that definition detection kicks
in elsewhere, consistent with the rest of the spec.
@ljharb ljharb force-pushed the editorial-complete-property-descriptor branch from 31f5c30 to e490377 Compare June 23, 2020 20:04
@ljharb ljharb requested review from a team and removed request for bmeck and zenparsing June 23, 2020 20:04
…tor" (tc39#1241)

Previously a mixture of "complete property descriptor" and "fully
populated property descriptor" was used.

Also capitalise "Property Descriptor" so that definition detection kicks
in elsewhere, consistent with the rest of the spec.
@ljharb ljharb force-pushed the editorial-complete-property-descriptor branch from e490377 to 8f82b21 Compare June 23, 2020 20:05
@ljharb ljharb merged commit 8f82b21 into tc39:master Jun 23, 2020
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