Editorial: Tweak EnumerateObjectProperties informative definition #656
Conversation
| @@ -16175,16 +16175,16 @@ | |||
| for (let key of Reflect.ownKeys(obj)) { | |||
| if (typeof key === "string") { | |||
| let desc = Reflect.getOwnPropertyDescriptor(obj, key); | |||
| if (desc && !visited.has(key)) { | |||
| if (desc && !visited.has(key)) { | |||
There was a problem hiding this comment.
Why? it just makes the code in spec.html less readable.
There was a problem hiding this comment.
Agreed. @shvaikalesh See https://mathiasbynens.be/notes/ambiguous-ampersands for more info on why this is perfectly valid.
There was a problem hiding this comment.
@mathiasbynens, thanks for the article. I dropped this commit and will make another PR to unescape all unambiguous ampersands in spec.html.
| if (desc.enumerable) yield key; | ||
| } | ||
| const visited = new Set; | ||
| for (const key of Reflect.ownKeys(obj)) { |
There was a problem hiding this comment.
there's no tweak here. This change is not necessary at all and for consistency it is better to remain without change.
There was a problem hiding this comment.
@leobalter, thanks for the review. Is it 46b569d or f1883f5 that should be dropped?
There was a problem hiding this comment.
I'm just part of TC39, @bterlson is the one able to answer that.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
I am fine with this change. I don't see why this change isn't consistent though - @leobalter can you clarify?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
shvaikalesh
changed the title
* upstream/master: (301 commits) Editorial: remove mistakenly committed change to eval-related super errors Normative: Point to the latest version of UTR15 (tc39#681) Meta: update ecmarkup version dependency (tc39#682) Editorial: Update a variable name in Annex B.3.3.1 to refer to the proper variable in FunctionDeclarationInstantiation's body, and add warning comments by all the spec algorithms monkey-patched by Annex B so that future refactoring doesn't create broken/dangling references in Annex B. (tc39#677) Meta: update ecmarkup version dependency Editorial: dfn-ify 'goal symbol' Editorial: TypeArray -> TypedArray (tc39#675) Editorial: Correct a wrong cross-reference in Annex C (tc39#674) Editorial: tweak whitespace and articles (tc39#667) Editorial: Tweak EnumerateObjectProperties informative definition (tc39#656) Editorial: Rename variable (indx --> index) (tc39#658) Editorial: Fix reference in ScriptEvaluation (tc39#659) Editorial: Consistent `undefined or null` order (tc39#660) Editorial: Refactor Array.prototype.toLocaleString (tc39#662) Editorial: Tweak Array.prototype.join (tc39#661) Simplify description of CreateImmutableBinding (tc39#654) Normative: Resolve template argument references (tc39#609) Editorial: Refactor Array.prototype.join (tc39#638) Editorial: contains --> Contains in ModuleListItem SS Normative: Allow initializers in ForInStatement heads (tc39#614) ...
protoName-->protoKey, becausekey).continueforsymbols (less indentation, more obvious what keys are skipped).constinstead oflet.