use rest parameters instead of arguments #1524
Conversation
|
yes |
| */ | ||
| issueStatus(title, args) { | ||
| issueStatus(title, ...args) { | ||
| if (title === 'status' || title === 'statusEnd') { | ||
| this.emit(title, args); |
There was a problem hiding this comment.
wouldn't this stick all current args in an array? will that impact anything outside of changed below?
There was a problem hiding this comment.
yes. Kind of confusingly args was previously an array, and event consumers are expecting a single array from the event, but that made it awkward to call when switching everything else to rest params, so it was easiest to just have varargs and group them up into an array here.
| return Log._logToStdErr(title, Array.from(arguments).slice(1)); | ||
| static log(title, ...args) { | ||
| Log.events.issueStatus(title, ...args); | ||
| return Log._logToStdErr(title, args); |
There was a problem hiding this comment.
_logToStdErr doesn't seem to have been changed at all did it already support this?
There was a problem hiding this comment.
_logToStdErrdoesn't seem to have been changed at all did it already support this?
Maybe I'm misunderstanding you, but isn't this right? args is an array of all but the first argument, equivalent to the old Array.from(arguments).slice(1))?
There was a problem hiding this comment.
Oh, no I just misread the diff thinking it didn't have a second argument at all, disregard :)
| console.log = function() { | ||
| ConsoleQuieter._logs.push({type: 'log', args: arguments, prefix: opts.prefix}); | ||
| console.log = function(...args) { | ||
| ConsoleQuieter._logs.push({type: 'log', args: args, prefix: opts.prefix}); |
There was a problem hiding this comment.
nit: can just do {type: 'log', args,
now possible by dropping support for node older than v6
brendankenny
force-pushed
the
restparams
branch
from
4dd1150
to
46d23dd
Compare
| Log.events.issueStatus(title, arguments); | ||
| return Log._logToStdErr(title, Array.from(arguments).slice(1)); | ||
| static log(title, ...args) { | ||
| Log.events.issueStatus(title, ...args); |
There was a problem hiding this comment.
There is one difference with the previous code block.
...args doesn't contain title whereas arguments did.
to fix you should do issueStatus(title, [title, ...args])
There was a problem hiding this comment.
ugh, I looked at this, thought it was terrible but had to be maintained, then promptly forgot about it, apparently. Thanks :)
| } | ||
|
|
||
| static verbose(title) { | ||
| static verbose(title, ...args) { | ||
| Log.events.issueStatus(title); |
There was a problem hiding this comment.
See comment above about arguments, ...args inside the function will be an empty []
There was a problem hiding this comment.
See comment above about arguments, ...args inside the function will be an empty []
I don't think that applies here since it's only doing Log.events.issueStatus(title), so it gets an empty array on master already.
Honestly I have no idea why it's doing issueStatus here with no args anyways. It doesn't make much sense.
There was a problem hiding this comment.
looks like it was a bug introduced in 71195e4#diff-e5b0c339f7ea6da4ec03ff079a28288cL100 I'll fix.
also reduces rest/spread repetitions
|
updated (see second commit). Also reduced some of the ridiculous repetitions of rest/spread/rest/spread :) |
|
😴 + ⛳️ + 🐏 s |
* use rest parameters instead of arguments now possible by dropping support for node older than v6 * fix status event payload also reduces rest/spread repetitions
enabled now that we have node v6 as the baseline.
sorry @ebidel, got to it first :)