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 feedback #67

Closed
michaelficarra opened this issue Apr 5, 2024 · 1 comment
Closed

editorial feedback #67

michaelficarra opened this issue Apr 5, 2024 · 1 comment

Comments

@michaelficarra
Copy link
Member

  1. In spec language, comparisons don't result in a Boolean value. You have to say something like "If a < b, then let _result_ be *true*".
  2. BigInts notation in spec text doesn't include the trailing n. 0n should just be 0.
  3. Regarding "If currentYieldingValue equal to end, " and "if step is zero and start is not equal to end": use "is"/"is not" or "="/"≠" to compare values as defined in various editorial changes for comparisons ecma262#2877 (comment).
  4. Use "Let ... be ..." and "Set ... to ...", never "Set ... be ...".
  5. No need to introduce _iterator_ before returning it, just return the result of the AO call directly.
  6. The URL in the note points to the wrong place. https://tc39.es/proposal-Number.range/playground.html should be https://tc39.es/proposal-iterator.range/playground.html.
  7. I would rename option to something like optionsOrStep.
  8. I don't like that ifIncrease is shadowed. It looks like you can just get rid of the outer binding.
  9. Why can't you just inline CreateNumericRangeIterator into Iterator.range? I don't see any reason to do a type test, branch, and then completely defer to an AO.
  10. CreateNumericRangeIterator doesn't seem to have its return type declared.
  11. I don't think you need to introduce "NumericRangeIterator Object". What's the purpose?
  12. "NOTE: Prevent value overflow." should be its own step, referring to the next/previous step, and include more explanation.

Use ecmarkup's --lint-spec and --strict options to catch some of these for you. Make sure you update to the latest version.

@Jack-Works
Copy link
Member

Why can't you just inline CreateNumericRangeIterator into Iterator.range? I don't see any reason to do a type test, branch, and then completely defer to an AO.

I don't think you need to introduce "NumericRangeIterator Object". What's the purpose?

I introduce NumericRangeIterator because I expect there will be more types of range in the future, for example, Temporal/Date range and I hope they'll have different object types.

All other reviews are fixed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants