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

Normative: rename to Iterator.range #59

Merged
merged 4 commits into from
Dec 5, 2022
Merged

Normative: rename to Iterator.range #59

merged 4 commits into from
Dec 5, 2022

Conversation

Jack-Works
Copy link
Member

API Before (in TypeScript):

var Number: {
    range(start: number, end: number, stepOrOptions: number | NumericRangeOptions<number>): IterableIterator<bigint>
}
var BigInt: {
    range(start: bigint, end: bigint | Infinity, stepOrOptions: bigint | NumericRangeOptions<bigint>): IterableIterator<bigint>
}

API now:

var Iterator: {
    range(start: number, end: number, stepOrOptions: number | NumericRangeOptions<number>): IterableIterator<number>
    range(start: bigint, end: bigint | Infinity, stepOrOptions: bigint | NumericRangeOptions<bigint>): IterableIterator<bigint>
}

@codecov-commenter
Copy link

Codecov Report

Merging #59 (b1932a4) into main (aa6d7d6) will increase coverage by 0.60%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main       #59      +/-   ##
===========================================
+ Coverage   99.39%   100.00%   +0.60%     
===========================================
  Files           1         1              
  Lines         165       145      -20     
  Branches       57        55       -2     
===========================================
- Hits          164       145      -19     
+ Misses          1         0       -1     
Impacted Files Coverage Δ
polyfill.js 100.00% <100.00%> (+0.60%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

<emu-clause id="sec-iterator.range">
<h1>Iterator.range ( _start_, _end_, _option_ )</h1>
<emu-alg>
1. If _start_ is a Number, return ? CreateNumericRangeIterator(_start_, _end_, _option_, ~number-range~).
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 this coerce start and end to a primitive first, like every other builtin api?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we coerce it to a primitive, we will lose the possibility to add more overload in the future, for example, supporting a Date range.

// for example:
Iterator.range(new Date('Jan 1 2020'), new Date(), { step: '1 day' })

Copy link
Member

Choose a reason for hiding this comment

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

That seems like something more appropriate to put on Date (or better, on Temporal) than in this function.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated
1. If _type_ is ~number-range~, let _primitiveType_ be *"number"*, else let _primitiveType_ be *"bigint"*.
1. Assert: Type(_start_) is _primitiveType_.
1. If _type_ is ~bigint-range~, let _zero_ be *0n*<sub>ℤ</sub>, else let _zero_ be *0*<sub>ℤ</sub>.
1. If _type_ is ~bigint-range~, let _one_ be *1n*<sub>ℤ</sub>, else let _one_ be *1*<sub>ℤ</sub>.
Copy link
Member

Choose a reason for hiding this comment

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

Another alternative approach is putting Z bar or R bar in a variable, and calling it like a function (also, i don’t think both of these should be Z bar?

Copy link
Member Author

Choose a reason for hiding this comment

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

what is "Z bar" or "R bar"? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

is "Z bar", eg

spec.emu Show resolved Hide resolved
1. Let _step_ be ? Get(_option_, "step").
1. Let _inclusiveEnd_ be ToBoolean(? Get(_option_, "inclusive")).
1. Else if Type(_option_) is _primitiveType_, let _step_ be _option_.
1. Else, throw a *TypeError* exception.
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, I’d expect the options to be coerced ToObject when it’s non-nullish, rather than throwing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

I guess maybe there's a consistency argument (though I'm not sure with what?) but it seems like that would generally be unintentional.

Copy link
Member Author

Choose a reason for hiding this comment

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

about coerce option, it's OK to me, but since I decided to not coerce start, I think it will be more consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Let me step back - what does Temporal do with options bags? We should at least be consistent with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

alrighty, fair enough. let's inline those same AOs, then, and use them.

Copy link

Choose a reason for hiding this comment

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

Temporal does what ECMA-402 does (at least, what ECMA-402 does for new APIs; there are some older Intl APIs that coerce primitives to objects). See tc39/ecma402#480

@zloirock
Copy link
Contributor

zloirock commented Dec 2, 2022

Iterator.range(new Date('Jan 1 2020'), new Date(), { step: '1 day' })

H̶̢͈̺̲͗͒̈́̈̿͆̋̋͘͝͝ơ̵͓̼̈́̏͑͊̈́͋̈́͒̕͘ļ̶̯̫̭̣̞̙̖̝͎͋̓̄͂͛̐͐̕ŷ̷̨̡̰̘̝͍̻̬͖̰̫͉̣̻̔̀͊͌̃̔̎͛́͛́̑͜͠ ̶̲̫͎̮̍̀́̔s̷̨̨̨̞̘̪̫͇͙͈̫͇̟̣̲̊́̀ͅͅh̸̡̧͕̱͖͙͎̮̐͆̌́̀͌̽̆͛͛̑͛̅̔̕͝į̴̫͈͉̩̺̹̤̹͚̖͇̰̊̊͋͌͑̃̈́̿͆͘̕͝t̸̨̨̛͇̘̠̞̼̏̈́̏̆̀̑̒̔̔͋̽͋̅̀.̴̢̤̖̀̐͂͂̊̂͆͒̇̆͐.̸̼̼̼͍̜͙̗͗̂̓̃̌̍͒̅͛͛̃͐̕̕̕͜͠.̷̡̢͚͙͇̬̳̦̫̫̓́̀̎͜ͅͅ

@zloirock
Copy link
Contributor

zloirock commented Dec 2, 2022

It's OK to join number and bigint versions of this method, but consider adding overloads for extra not similar types - it's a kind of god function...

@Jack-Works
Copy link
Member Author

just leave the possibility. not going to do it in the current proposal

@Jack-Works
Copy link
Member Author

I'll merge this first, but this is not the final conclusion of renaming.

I'll ask the committee if they like the rename in the next presentation.

@Jack-Works Jack-Works merged commit bb980c9 into main Dec 5, 2022
@Jack-Works Jack-Works deleted the iterator.range branch December 5, 2022 03:06
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

Successfully merging this pull request may close these issues.

None yet

6 participants