Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Apparently `yargs` has boolean negation enabled by default (so an option called `do-thing` automatically has a opposite option `no-do-thing`). * No strict mode: Either the negative or positive variant (or both) can be specified as an option, and the value is stored as the positive-named property (`argv.doThing`). * Strict mode: The positive variant must be specified as an option, or else it'll complain that that the positive-named variant is an unknown arg.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
anomiex
left a comment
There was a problem hiding this comment.
I wonder if jetpack changelog needs non-strict as well. Or else going through and declaring any missing options, such as --filename-auto-suffix for jetpack changelog add.
Other than that, LGTM.
Good catch. |
Proposed changes
This one's a bit silly, but I was trying to use
--with-depsinstead of--depswhen building, and it would happily build with no errors, but of course didn't build deps. By switching yargs tostrictby default, it now shows the help page when there's an invalid arg passed.Another common error is trying to use
--filterwith thetestcommand, which isn't supported.Some commands need option passthrough. Both
composerandpnpmalready override with.strict( false ). I addedchangeloganddockeras well.There's alsoIt turns outdocker, which can use--, but those are handled differently inargv._.dockeralso needs passthrough, as it does things likejetpack docker wp plugin --activate.Note that strict mode doesn't like negative (
no-*) options by default. I could have setboolean-negationand update the JS vars, but I decided to just define the positive options as hidden.Related product discussion/links
Does this pull request change what data or activity we track or use?
Testing instructions
Test various
jetpackcommands with valid and invalid flags, e.g.:jetpack build plugins/crm --deps: builds with depsjetpack build plugins/crm --with-deps: shows helpjetpack pnpm --version: shows pnpm versionjetpack composer --version: shows composer versionjetpack test js packages/connection --filter=asdf: shows helpjetpack docker phpunit jetpack -- --filter=Jetpack_Components_Test: runs filtered set of tests