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

feat(range): accept one argument #4360

Merged
merged 1 commit into from
Jan 30, 2019

Conversation

arpadvas
Copy link
Contributor

@arpadvas arpadvas commented Nov 19, 2018

Description:

Related issue (if exists):
#4332

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7665

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 96.808%

Totals Coverage Status
Change from base Build 7664: -0.2%
Covered Lines: 5246
Relevant Lines: 5419

💛 - Coveralls

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Jan 7, 2019
@benlesh
Copy link
Member

benlesh commented Jan 7, 2019

Tentatively, I'm okay with this addition, but:

  1. It needs to be discussed with the core team
  2. It will have to wait for a minor release. (not a patch).

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Jan 16, 2019
@benlesh
Copy link
Member

benlesh commented Jan 16, 2019

NOTE: This is a possible breaking change for JavaScript users doing something like: range(-5) and relying on counting up to "zero" which is undefined. (I haven't tested this, but it seems very corner-casey anyway)

@arpadvas
Copy link
Contributor Author

NOTE: This is a possible breaking change for JavaScript users doing something like: range(-5) and relying on counting up to "zero" which is undefined. (I haven't tested this, but it seems very corner-casey anyway)

True. range(-5) doesn't work. Shall I update the PR so when range(-5) is applied it'd count up to zero (-4, -3, -2, -1, 0)?

@cartant
Copy link
Collaborator

cartant commented Jan 17, 2019

Shall I update the PR

Hmm. But range(-5) counting up to zero would be nothing like lodash - AFAICT - so my vote would be to not have it count up to zero.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

@benlesh benlesh merged commit a388578 into ReactiveX:master Jan 30, 2019
@arpadvas arpadvas deleted the range-accept-only-one-argument branch January 30, 2019 07:03
@lock lock bot locked as resolved and limited conversation to collaborators Mar 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants