Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upReduce the indirection through _outgoing in _request #107
Conversation
| const params = typeof args[0] === 'object' && args.shift() | ||
| const outgoing = { |
This comment has been minimized.
This comment has been minimized.
paulmelnikow
Dec 30, 2017
Author
Collaborator
This code was moved into _makeRequest() below, with a few changes.
|
I noticed that too. Those 4 params were reverse engineered from having read the code and deduced (incorrectly) what was actually used. In reality, this is a set of options for |
|
This all looks like sensible replumbing. I've checked it out in place of the published npm package, and all of my tests act as I'd expect. There's some slightly different behaviour in failing tests when all run together, but that's looking to be the peculiarities of my repo and caused by #93 (in this case, different tests failing) and hopefully #106 will help to resolve it. Unless you have any concerns, I'm happy to merge. |
|
Yea, sounds great. Let's discuss the |
paulmelnikow commentedDec 30, 2017
•
edited
This isn't an obvious win, though it should facilitate further cleanup in
_makeRequest().@Fishbowler I'm curious, currently the documentation for the
paramspassed to each command calls out four supported parameters. The implementation, in most cases, actually passes through unknown parameters torequest(). Do you think that's desirable? If we were to change the implementation so it only worked with the documented parameters, do you think that would be a breaking change?