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.

Sign up for GitHub

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: Tweak EnumerateObjectProperties informative definition #656

Merged
merged 5 commits into from
Aug 12, 2016
Merged

Editorial: Tweak EnumerateObjectProperties informative definition #656

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4890aec
add missing semicolon
shvaikalesh Aug 10, 2016
7e71f79
improve variable naming consistency
shvaikalesh Aug 10, 2016
46b569d
early return for symbols
shvaikalesh Aug 12, 2016
f1883f5
prefer const over let
shvaikalesh Aug 12, 2016
aff0edd
new Set --> new Set()
shvaikalesh Aug 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions spec.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16171,20 +16171,19 @@ <h1>EnumerateObjectProperties (_O_)</h1>
<p>The following is an informative definition of an ECMAScript generator function that conforms to these rules:</p>
<pre><code class="javascript">
function* EnumerateObjectProperties(obj) {
let visited = new Set;
for (let key of Reflect.ownKeys(obj)) {
if (typeof key === "string") {
let desc = Reflect.getOwnPropertyDescriptor(obj, key);
if (desc && !visited.has(key)) {
visited.add(key);
if (desc.enumerable) yield key;
}
const visited = new Set();
for (const key of Reflect.ownKeys(obj)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t we use const over let throughout this code sample? None of the lets are reassigned.

Copy link
Member Author

@shvaikalesh shvaikalesh Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It is mostly a matter of code style. Some might argue that consts are for declarations that will never ever be reassigned, like number constants or requires. And lets are for any other declarations: this keeps diffs cleaner.

Lets make them consts to reflect how most people write ES201* these days.

Copy link
Member

Choose a reason for hiding this comment

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

there's no tweak here. This change is not necessary at all and for consistency it is better to remain without change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@leobalter, thanks for the review. Is it 46b569d or f1883f5 that should be dropped?

Copy link
Member

Choose a reason for hiding this comment

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

I'm just part of TC39, @bterlson is the one able to answer that.

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 it's ok to change it to use const, since it's never reassigned. I'd also want to see new Set() while we're at it :-)

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with this change. I don't see why this change isn't consistent though - @leobalter can you clarify?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer this example being consistent (the same) along the new releases.

  • replacing let w/ const will make it valid to replace const w/ let in any near future as we don't enforce any particular use.

Copy link
Member

Choose a reason for hiding this comment

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

We do enforce a particular use in the spec at least - whatever I feel like 😈

My feelings may change (I have in fact become far less likely to use const) but for now I think this helps and I promise I won't change it back to let any time soon.

Copy link
Member Author

@shvaikalesh shvaikalesh Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@ljharb addressed in aff0edd, thanks for the feedback.

if (typeof key === "symbol") continue;
const desc = Reflect.getOwnPropertyDescriptor(obj, key);
if (desc && !visited.has(key)) {
Copy link
Contributor

@claudepache claudepache Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why? it just makes the code in spec.html less readable.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. @shvaikalesh See https://mathiasbynens.be/notes/ambiguous-ampersands for more info on why this is perfectly valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mathiasbynens, thanks for the article. I dropped this commit and will make another PR to unescape all unambiguous ampersands in spec.html.

visited.add(key);
if (desc.enumerable) yield key;
}
}
let proto = Reflect.getPrototypeOf(obj)
const proto = Reflect.getPrototypeOf(obj);
Copy link
Member

Choose a reason for hiding this comment

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

this ; is the only part I agree we should merge. The remaining are unnecessary as TC39 does not advise using const over let or against it.

Copy link
Member Author

@shvaikalesh shvaikalesh Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Any JavaScript code contains opinionated parts: this function will be just fine without semicolons and strict equals. Does TC39 advise against == and ASI?

Copy link
Member

@ljharb ljharb Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

TC39 isn't advising one over the other in either case. Since both are equivalent, and the community tends to prefer const here, I think that's the better choice.

if (proto === null) return;
for (let protoName of EnumerateObjectProperties(proto)) {
if (!visited.has(protoName)) yield protoName;
for (const protoKey of EnumerateObjectProperties(proto)) {
if (!visited.has(protoKey)) yield protoKey;
}
}
</code></pre>
Expand Down