feat(limitTo): ignore limit when undefined #10510
Conversation
|
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. |
marcin-wosinek
force-pushed
the
master
branch
from
cc70067
to
c09dea9
Compare
|
CLAs look good, thanks! |
| @@ -15,7 +15,8 @@ | |||
| * @param {string|number} limit The length of the returned array or string. If the `limit` number | |||
| * is positive, `limit` number of items from the beginning of the source array/string are copied. | |||
| * If the number is negative, `limit` number of items from the end of the source array/string | |||
| * are copied. The `limit` will be trimmed if it exceeds `array.length` | |||
| * are copied. The `limit` will be trimmed if it exceeds `array.length`. If `limit` is undefined, | |||
| * the input will be left unchanged. | |||
There was a problem hiding this comment.
IMO, "returned" is more appropriate than "left" (since the input is left unchanged in any case).
|
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 But "functionality-wise" it should be OK. BTW, the commit message and breaking change notice are slightly inaccurate: It is not only |
marcin-wosinek
force-pushed
the
master
branch
from
c09dea9
to
3c0d8e4
Compare
|
@gkalpak thanks for the review |
marcin-wosinek
force-pushed
the
master
branch
2 times, most recently
from
7d48430
to
f1a6390
Compare
|
I'm lost with the build result. I get: from job 1. |
| 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([]); |
There was a problem hiding this comment.
Nit picking: Could add a test for '0' (string) as well.
|
@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: |
marcin-wosinek
force-pushed
the
master
branch
2 times, most recently
from
d2e50bd
to
b66a1fe
Compare
|
LGTM |
marcin-wosinek
force-pushed
the
master
branch
from
b66a1fe
to
a9ba807
Compare
BREAKING CHANGE: limitTo changed behavior when limit value is invalid. Instead of returning empty object/array it returns unchanged input. Closes angular#10484
marcin-wosinek
force-pushed
the
master
branch
from
a9ba807
to
ea5085c
Compare
BREAKING CHANGE: limitTo changed behavoir when limit value is undefined.
Instead of returning empty object/array it returns non-changed input.
Closes #10484