Infinity and early conditions#11
Infinity and early conditions#11customcommander wants to merge 7 commits intojonschlinkert:masterfrom
Conversation
|
|
||
| var max = str.length * num; | ||
|
|
||
| if (max === Infinity) { |
There was a problem hiding this comment.
Nice work. Please move this check to be just after the string check (line 46 or so) and change it to if (num === Infinity).
There was a problem hiding this comment.
The reason I put it on that line was to leverage the implicit "parseFloat()" when str.length and num are multiplied together e.g.
5 * Infinity;
//=> Infinity
5 * 'Infinity';
//=> Infinity
5 * ' Infinity ';
//=> InfinityThat was also motivated by some of the tests that seem to suggest that num can be given as a string too. I'm happy to make the change but should I still care about the case of Infinity as a string then?
By the way it seems that if num can be given as a string then the two shortcuts at L50,51 are most likely ignored. Should this be addressed?
|
@customcommander nice work! I only had the one comment, everything else is great. thank you! |
This pull request has a few things going on:
varandconst. No need to have both so replaced withconst.InfinityInfinity
The native
String#repeatmethod does throw an error whenInfinityis given as a parameter. So this changes aligns with the native behaviour. Also there's an issue with howInfinityworks right now: with the same string to repeat, it will produce a different string:This didn't seem right to me so took the liberty to throw an error instead.