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
refactor: harden plugins against unknown options #804
Conversation
This reworks the plugin API such that: - Unable to register a command with unknown wrap-options - `TypeError` raised for wrap-option type mistakes - Remove the `overWrite` option (it's unused, probably safest to not expose for now) - `cmdOptions` defaults to `null` instead of `false` for type consistency (no change to default behavior) - Move `pipeMethods` logic into `_register`, since it makes more sense there This is not expected to have any effect on existing plugins.
Codecov Report
@@ Coverage Diff @@
## master #804 +/- ##
==========================================
+ Coverage 95.47% 95.49% +0.01%
==========================================
Files 34 34
Lines 1260 1265 +5
==========================================
+ Hits 1203 1208 +5
Misses 57 57
Continue to review full report at Codecov.
|
wrapOutput: true, | ||
overWrite: false, | ||
unix: true, |
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.
Looking at the source, unix
is only used by exec
, because exec
args differ from the other commands. I think now would be a good chance to improve this.
It makes sense to expose the ability to skip manipulating the args, so let's rename the option to make that clearer. Something like preserveArgs
?
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 was planning to hide the unix
option in a later PR. Renaming is ok too, but I don't see a need to expose it (we already offer ways to disable globbing and option parsing, which are the most expensive argument manipulations).
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.
Ok, that's fine.
LGTM |
This reworks the plugin API such that:
TypeError
raised for wrap-option type mistakesoverWrite
option (it's unused, probably safest to notexpose for now)
cmdOptions
defaults tonull
instead offalse
for typeconsistency (no change to default behavior)
pipeMethods
logic into_register
, since it makes more sensethere
This is not expected to have any effect on existing plugins.