Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Replace
push.apply
with dedicated function #12361base: main
Are you sure you want to change the base?
Replace
push.apply
with dedicated function #12361Changes from 4 commits
f7455ad
4257266
c088b63
c00243a
62c4888
51bc1e1
c237a8e
6372880
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick on semantics, but this time I do think it actually matters. When I first glanced at this code I expected the order of the arguments to be reversed:
addAll(target, source)
.This is largely because I am comparing this to similar functions of
target.push(...source)
ortarget.concat(source)
etc. In all the builtin array functions the "result" is on the left and the "source" is on the right.Another way I think about it is that this is replacing the syntax
[...target, ...source]
to put all the target elements first and the source elements second. So the target variable should be the first argument. Even in this example you're doingaddAll([ 3, 4, 5 ], [ 0, 1, 2 ]) = [ 0, 1, 2, 3, 4, 5 ]
(Maybe "source" and "target" are the wrong names in the first place)
I think swapping them also aligns better with the idea that the "source" could be
null
,undefined
or[]
. Then the first argument is always defined even if the second only sometimes is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also hesitated here. Leaving the source as the last parameter would allow omitting it, as in
addAll(target);
which is probably not ideal.
But in general, I'm open to changing it, and embarassingly have to point out that commit c00243a ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my all-time favorite presentation on API design:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped the parameters. Fortunately, someone posted the Regex for that, which is to ...
replace
(addAllToArray)\(([^,]+), *([^,]+)\)
with
$1($3, $2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I like the name
addAll
. I don't feel it's descriptive enough to understand what it does without looking at the documentation or the function itself.addAllToArray
or maybecombineArrays
could be better? Or, given my previous comment on this topic with this essentially doing whatArray.concat
does, maybeconcatInto
orconcatIntoArray
would be more accurate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No really strong opinions, just things that I thought about:
pushAll
, but I wanted to emphasize that it does exactly notpush
...concat
could easily be confused withArray.concat
, and (as discussed in the other PR), that has a completely different semantic: People might expect it to return a concatenation. I think it's important to make clear that it operates in-place, and on the given target arraycombine...
could be OK (maybe people would expect it to return something there, but not necessarily)Maybe something like
appendAll
orappend(All?To?)Array
or so ...?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to
addAllToArray
- sounds reasonable and is long and clear enough for a global function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a nitpick but in general I think we should avoid single letter variable names outside the looping variables
i
,j
,k
, and sometimesl
.Maybe it's just personal preference but I find this a little more readable even if it is more verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I nearly espected that, and can change it accordingly.
But regarding the variable name
1
: Here is an animated GIF ofl
and1
, switching every second:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to
sourceLength
andtargetLength