/angular.js

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(limitTo): ignore limit when undefined #10510

Closed
wants to merge 1 commit into
base: master
from

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
3 participants
@marcin-wosinek @googlebot @gkalpak
@marcin-wosinek
Contributor

marcin-wosinek commented Dec 17, 2014

BREAKING CHANGE: limitTo changed behavoir when limit value is undefined.
Instead of returning empty object/array it returns non-changed input.

Closes #10484
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Dec 17, 2014

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot commented Dec 17, 2014

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added the cla: no label Dec 17, 2014

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Dec 17, 2014

CLAs look good, thanks!

googlebot commented Dec 17, 2014

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 17, 2014

Show outdated Hide outdated src/ng/filter/limitTo.js
Show outdated Hide outdated src/ng/filter/limitTo.js
test/ng/filter/limitToSpec.js
expect(limitTo(items, 'null')).toEqual(items);
expect(limitTo(items, 'undefined')).toEqual(items);
expect(limitTo(items, null)).toEqual(items);
expect(limitTo(items, undefined)).toEqual(items);

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

There should also be a test with 0 (to ensure it works as expected).

UPDATE: Has been added.
Copy URL
@gkalpak

gkalpak Dec 18, 2014

Member

There should also be a test with 0 (to ensure it works as expected).

UPDATE: Has been added.
test/ng/filter/limitToSpec.js
expect(limitTo(str, 'null')).toEqual(str);
expect(limitTo(str, 'undefined')).toEqual(str);
expect(limitTo(str, null)).toEqual(str);
expect(limitTo(str, undefined)).toEqual(str);

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

There should also be a test with 0 (to ensure it works as expected).

UPDATE: Has been added.
Copy URL
@gkalpak

gkalpak Dec 18, 2014

Member

There should also be a test with 0 (to ensure it works as expected).

UPDATE: Has been added.
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

I have left a couple of minor comments inline.

Additionally, I believe a little refactoring is in place: it would be better (i.e. more maintainable/performant) to do the NaN check sooner in the code (and return the input itself), so we can (1) "fail fast" and (2) avoid some of the else blocks further down.

But "functionality-wise" it should be OK.

BTW, the commit message and breaking change notice are slightly inaccurate: It is not only undefined but any invalid value, right ?
Member

gkalpak commented Dec 18, 2014

I have left a couple of minor comments inline.

Additionally, I believe a little refactoring is in place: it would be better (i.e. more maintainable/performant) to do the NaN check sooner in the code (and return the input itself), so we can (1) "fail fast" and (2) avoid some of the else blocks further down.

But "functionality-wise" it should be OK.

BTW, the commit message and breaking change notice are slightly inaccurate: It is not only undefined but any invalid value, right ?
@marcin-wosinek

This comment has been minimized.

Show comment
Hide comment
@marcin-wosinek

marcin-wosinek Dec 18, 2014

Contributor

@gkalpak thanks for the review
Contributor

marcin-wosinek commented Dec 18, 2014

@gkalpak thanks for the review
@marcin-wosinek

This comment has been minimized.

Show comment
Hide comment
@marcin-wosinek

marcin-wosinek Dec 18, 2014

Contributor

I'm lost with the build result. I get:

No output has been received in the last 10 minutes, this potentially indicates 
a stalled build or something wrong with the build itself.

The build has been terminated

from job 1.
Contributor

marcin-wosinek commented Dec 18, 2014

I'm lost with the build result. I get:

No output has been received in the last 10 minutes, this potentially indicates 
a stalled build or something wrong with the build itself.

The build has been terminated

from job 1.
test/ng/filter/limitToSpec.js
expect(limitTo(items, null)).toEqual([]);
expect(limitTo(items, undefined)).toEqual([]);
it('should return an empty array when X = 0', function() {
expect(limitTo(items, 0)).toEqual([]);

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

Nit picking: Could add a test for '0' (string) as well.
Copy URL
@gkalpak

gkalpak Dec 18, 2014

Member

Nit picking: Could add a test for '0' (string) as well.
test/ng/filter/limitToSpec.js
});
it('should return an empty string when X = 0', function() {
expect(limitTo(str, 0)).toEqual("");

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

Nit picking: Could add a test for '0' (string) as well.
(I know it was like this before, but single-quotes would also be preferable to double-quotes for consistency.)
Copy URL
@gkalpak

gkalpak Dec 18, 2014

Member

Nit picking: Could add a test for '0' (string) as well.
(I know it was like this before, but single-quotes would also be preferable to double-quotes for consistency.)

This comment has been minimized.

Show comment
Hide comment
@marcin-wosinek

marcin-wosinek Dec 18, 2014

Contributor

Added
Copy URL
@marcin-wosinek

marcin-wosinek Dec 18, 2014

Contributor

Added
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

@marcin-wosinek: It is a flake (not related to the build) on SauseLabs only. I restarted the job, but since the tests passed on BrowserStack, it should be OK.

I left two "nit-picky" comments on the tests, but other than that it looks good to me.

(There's a typo in the BREAKING CHANGE notice btw: behavoir --> behavio[u]r)
Member

gkalpak commented Dec 18, 2014

@marcin-wosinek: It is a flake (not related to the build) on SauseLabs only. I restarted the job, but since the tests passed on BrowserStack, it should be OK.

I left two "nit-picky" comments on the tests, but other than that it looks good to me.

(There's a typo in the BREAKING CHANGE notice btw: behavoir --> behavio[u]r)
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

LGTM
Member

gkalpak commented Dec 18, 2014

LGTM
@marcin-wosinek
feat(limitTo): ignore limit when invalid 
BREAKING CHANGE: limitTo changed behavior when limit value is invalid.
Instead of returning empty object/array it returns unchanged input.

Closes #10484
Loading status checks…
ea5085c

@gkalpak gkalpak referenced this pull request Dec 20, 2014

Closed

refactor(limitTo): no need for all those checks if we use slice #10537

@petebacondarwin petebacondarwin closed this in a3c3bf3 Dec 21, 2014

@marcin-wosinek marcin-wosinek referenced this pull request Dec 21, 2014

Closed

limitTo: Ignore `limit` when `undefined` #10484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment