Fail fast if user provides unknown flags #8
Comments
|
How do you guys feel about switching from minimist to commander for the options parsing? I think this will help with #3, #8, and #9. I think we can have better "fail fast" functionality and more descriptive code by moving to commander. It should remove a lot of the manually verification of options. I'm working on an idea for #3 with commander, but I wanted to see if there were any objections before I put together a pull request... |
|
Commander might be the industry's most used package for this purpose, but it's actually full of old code and bloated with unnecessary stuff. Let's go with args (like now-serve and list)! 😊 |
|
Is anyone interested in switching the project to args? Because that would it make it a lot easier for me to merge now-serve into now. If not, I'll tacke it! |
|
@leo I worked out a proof of concept locally for #3, but it's independent of the args parser. So I haven't looked yet migrating to args. I don't want to slow you down if you're ready to make that migration. I had a question fail fast (this issue and #8) and args. What I saw with how we use minimist and args is that each command checks the flags when it needs them. I guess we can move all of those controls the start of each script to enforce the fail fast. However, the default behavior of commander is to throw an error on invalid args, which I thought might be interesting for these issues. Anyways, I'm not sure if this is open to discussion, but I thought it was worth making that point. In any case, I'm wondering if it would be worth decoupling the args parsing from the actual scripts? In theory this would allow us to write tests for the command line parsing (while mocking the lib). IMO this would add a lot of quality and add separation of concerns to the design. To answer you question, I would be happy to look at #3 and the fail fast issues. But it seems migrating the args parser needs to be done first. If your ready to do that, you should go for it because I won't have many hours in the next days. If you have higher priorities, I can start working on it when I find those hours. Let me know what works best for you. We can chat more about these points on slack too... |
|
@leo I've been looking at this again. I'm looking at how it could work with args, but I don't see an obvious way to catch unknown flags. I see minimist has the Am I missing something? Or is this something we would want to add to args? |
|
@codyzu Looks like a great addition to me! Also: leo/args#45 |
|
We started using commander, but for example we didn't have the ability to override quitting to make it wait on the update checker. It's very opinionated and tends to be quite inflexible sometimes. |
|
tl;DR: I'd rather re-implement checking for unknown flags rather than trade off flexibility |
|
I assumed there must be a back story behind not using commander. That makes sense. Thanks @leo, looks like leo/args#45 is the way to go. I'll look at that first... |
|
Replaced by #1171 |
leo
pushed a commit
that referenced
this issue
Nov 8, 2018
Otherwise it's just `[object Object]` which is useless.
leo
added a commit
that referenced
this issue
Nov 8, 2018
* Removed external providers * Renamed sh provider * Removed serverless stuff * Fixed paths * Properly pass token * Fixed paths * Check for token correctly * Remove useless properties * Fixed unit tests * Keep certain things * Adjusted remaining parts * Fixed login * Remove user properties that are not needed * Store `platformVersion` for teams * Store `platformVersion` for users * Delete team order when logging out * Made team commands work with tiny config * Load data dynamically * Made billing command show correct context * Fixed remaining occurences of context name * Fixed remaining pieces * Test CI * Clean strings when testing * Better error check * Render correct information when asking for credit card * Migrate from objects to strings * Better migration message and keep tips * Remove the old property * Use note for migration message * Don't show spinner for loading missing data * Allow for current team or user to be deleted * Two deploy files * Consume context name correctly * Removed deployment types from new deployer * Also check for bigger than 1 * Better file names * Added upgrade message for legacy deployer * Make help work when logged out * Error if sub command doesn't exist when requesting help * Fixed wording * Support for version property added * Better handling * Removed useless props from deploy help * Don't show version warnings when rendering help * Fixed wording in readme * Migrate even if in the middleground * Make `now whoami` work * Deprecated support for deploying Git repo in latest deployer * Added usage information * Load config separately * Don't need local config on root * Correct status code for help test * Show error only in correct case * Properly error if path does not exist * Fixed types * Fixed remaining occurances of old config * Don't show warning when rendering help * Consider version when outputting help * Only error if path does not exist if it's not help * Remove testing logging * Don't error for no-verify * Stop logging * Fixed last test * Added missing semicolons * Fix indent * Fix indent again * Ran prettier over everything * Added missing types * Brought test hashes back to normal * Exit properly when deploying * Show clipboard note in gray and remove Node.js version * Completed usage information * Ensure `now whoami` only outputs the user * Don not save user data to config * Removed last traces of `user` and `.api` and `--team` work * Made `now upgrade` and `now downgrade` correctly render context * Fixed upgrade/downgrade URL for teams * Ability to load required data * Better file name * Corrected check for current scope * Don't render version warning when showing help * Keep polling for handlers * Render handlers * Removed useless file * Much better transpilation setup * Sweetened logging * Shortened time it takes to render ready * Support for error ready states * Make sure table is not wobbling * Show times for every handler * Attach env and build env * Don't pass useless stuff * Re-structured pipeline * Allow empty config * Do not support package.json config for new pipeline * Removed occurances of AWS * Drop useless packages * Removed useless file * Ensure the legacy pipeline is working * Test staging proxy with legacy pipeline * Adjust test * Stop testing staging proxy * Allow for anything * Pass `handlers` and `routes` to creation endpoint * Fixed tests * Revert "Fixed tests" This reverts commit e0d18a6. * Running tests should not be optional * Support for `-m` and `--meta` added * Support reading `meta` in local config * Allow reading `name` from local config * The `public` prop in local config should be considered * Handlers deployments should use `.nowignore` and nothing else * Allow handlers deployments without `handlers` and `routes` in the config * Locked legacy tests to legacy platform version * Support aliasing handlers deployments * Removed useless condition * Don't allow scaling handlers deployments * Don't show warning message when deploying single file * Fixed tests * Made `now inspect` work * No type for handlers deployments in list * Indicate whether a deployment is legacy in `now inspect` * Made `--force` work for handlers deployments * Do not document `--dotenv` for handlers (not supported) * Do not strip `hdl_` from handler IDs * Fixed for upgrading to latest platform * Better error for when `version` property is missing * Render platform version while deploying * Strip `hdl_` from handler IDs This reverts commit 750d38b. * Removed `https://` from handler list * Removed demo mock * Cleaner errors * Make times and erroring work properly * Print final deployment error * Removed useless promise * Prettified code * Put config utils into correct location * Moved even more config files * Removed useless directory * Removed last useless file * Fixed wrong paths * Fixed unit tests * Update deployment according to handler state, like the server-side loop * More robust deployment mechanism * Poll every 1.5 seconds * Prevent many requests * Show spinner while waiting for deployment to be ready * Render how long the deployment took * Avoid unnecessary repainting * Automatically remove useless `user` property from config when migrating * Fixed property names in `now inspect` * Render platform version for legacy pipeline * Shortened error messages * Support `regions` in the local config * Support for `--regions` added * Add metadata support for legacy deployments (#2) * Share handlers table between deploying and inspecting * Make `now inspect` work nicely * Renamed handlers to builds * Stop sending away description * Bare UI support for builds * Simper logic for rendering builds * Render output of builds * Indicate lambdas in a better way * Render size for build output * Do not show type for version 2 deployments * Fixed time output for `now inspect` * Don't handle BUILDS type * Allow for 100% non-config deployments * Add metadata support for now ls (#3) * Add metadata support for now ls So, we can do things like this: now ls -m key1=value1 -m key2=value2 * Better description * Fix wording * Added final newline * Add sentry (#4) * Revert "Add sentry (#4)" This reverts commit 851d1bd. * Only render build output if it was not copied * Made `now alias` work with latest staging proxy * Revert "Made `now alias` work with latest staging proxy" This reverts commit 16e8998. * Bumped deployments API to the latest version * Made `now rm` work * Do not print `version` warning for single files * Removed useless `fs-extra` dependency * Removed useless dependencies * Default binaries to Node 10 * Bumped Node.js in Circle CI to latest * Bumped Xcode to get latest Node.js for integration tests * Enabled HTTP/2 support * Removed useless code * Added integration test for builds * Bumped `fetch-h2` to the latest version * Avoid performing network request for rendering help * Render note when viewing latest help * Return status code `2` when exiting with help * Fixed test for usage information * Removed wrong text in usage info for Now 2.0 * Removed support for `--links` from v2 pipeline * migrated => upgraded * Added default routing for single files * Make `--token` work as expected * Better message for build errors * Prevent update notification * Prevent update notifications from showing * Only show migration message in debug output * Prevent flickering of state * Improved output * Removed useless assignment * Corrected padding * Less padding before state * Corrected links for global configuration * Fixed integration tests * Render region for Lambdas * Join regions in a better way * Ensure `now.json` and `.nowignore` (the latter worked anyways) are uploaded * Fix `build.env` in new deployment API call (#6) * JSON log the deployment body when debugging (#8) Otherwise it's just `[object Object]` which is useless. * Retry to fetch on error on follow mode (#5) * retry fetch on error on follow mode * improve logging * Fixed `--env` and `--build-env` CLI args (#7) * Fix `--env` and `--build-env` CLI args * Fall back to `undefined` * Ensure `now switch` lists active scope in the beginning * Removed useless code * Error if `env` or `build.env` have wrong types * Made `now inspect` look great * Fixed wrong protocol in URL * Made `now --team` work with users * Leave PHP out of integration test for now * Do not select PHP build * Fixed integration tests * Revert "12.0.0-canary.93" This reverts commit 70a0a59. * Revert "Revert "Add support for top-level "sh" auth"" This reverts commit 4273d62. * Revert "12.0.0-canary.92" This reverts commit 847c71e. * Revert "Add support for top-level "sh" auth" This reverts commit c493d65. * Revert "12.0.0-canary.91" This reverts commit 06c954f. * Revert "Added metadata support for `now inspect` (#1634)" This reverts commit 9567656. * Revert "12.0.0-canary.90" This reverts commit 966737b. * Revert "Added support for deployment metadata (#1604)" This reverts commit 6c1188a.
leo
added a commit
that referenced
this issue
Nov 8, 2018
* Removed external providers * Renamed sh provider * Removed serverless stuff * Fixed paths * Properly pass token * Fixed paths * Check for token correctly * Remove useless properties * Fixed unit tests * Keep certain things * Adjusted remaining parts * Fixed login * Remove user properties that are not needed * Store `platformVersion` for teams * Store `platformVersion` for users * Delete team order when logging out * Made team commands work with tiny config * Load data dynamically * Made billing command show correct context * Fixed remaining occurences of context name * Fixed remaining pieces * Test CI * Clean strings when testing * Better error check * Render correct information when asking for credit card * Migrate from objects to strings * Better migration message and keep tips * Remove the old property * Use note for migration message * Don't show spinner for loading missing data * Allow for current team or user to be deleted * Two deploy files * Consume context name correctly * Removed deployment types from new deployer * Also check for bigger than 1 * Better file names * Added upgrade message for legacy deployer * Make help work when logged out * Error if sub command doesn't exist when requesting help * Fixed wording * Support for version property added * Better handling * Removed useless props from deploy help * Don't show version warnings when rendering help * Fixed wording in readme * Migrate even if in the middleground * Make `now whoami` work * Deprecated support for deploying Git repo in latest deployer * Added usage information * Load config separately * Don't need local config on root * Correct status code for help test * Show error only in correct case * Properly error if path does not exist * Fixed types * Fixed remaining occurances of old config * Don't show warning when rendering help * Consider version when outputting help * Only error if path does not exist if it's not help * Remove testing logging * Don't error for no-verify * Stop logging * Fixed last test * Added missing semicolons * Fix indent * Fix indent again * Ran prettier over everything * Added missing types * Brought test hashes back to normal * Exit properly when deploying * Show clipboard note in gray and remove Node.js version * Completed usage information * Ensure `now whoami` only outputs the user * Don not save user data to config * Removed last traces of `user` and `.api` and `--team` work * Made `now upgrade` and `now downgrade` correctly render context * Fixed upgrade/downgrade URL for teams * Ability to load required data * Better file name * Corrected check for current scope * Don't render version warning when showing help * Keep polling for handlers * Render handlers * Removed useless file * Much better transpilation setup * Sweetened logging * Shortened time it takes to render ready * Support for error ready states * Make sure table is not wobbling * Show times for every handler * Attach env and build env * Don't pass useless stuff * Re-structured pipeline * Allow empty config * Do not support package.json config for new pipeline * Removed occurances of AWS * Drop useless packages * Removed useless file * Ensure the legacy pipeline is working * Test staging proxy with legacy pipeline * Adjust test * Stop testing staging proxy * Allow for anything * Pass `handlers` and `routes` to creation endpoint * Fixed tests * Revert "Fixed tests" This reverts commit e0d18a6. * Running tests should not be optional * Support for `-m` and `--meta` added * Support reading `meta` in local config * Allow reading `name` from local config * The `public` prop in local config should be considered * Handlers deployments should use `.nowignore` and nothing else * Allow handlers deployments without `handlers` and `routes` in the config * Locked legacy tests to legacy platform version * Support aliasing handlers deployments * Removed useless condition * Don't allow scaling handlers deployments * Don't show warning message when deploying single file * Fixed tests * Made `now inspect` work * No type for handlers deployments in list * Indicate whether a deployment is legacy in `now inspect` * Made `--force` work for handlers deployments * Do not document `--dotenv` for handlers (not supported) * Do not strip `hdl_` from handler IDs * Fixed for upgrading to latest platform * Better error for when `version` property is missing * Render platform version while deploying * Strip `hdl_` from handler IDs This reverts commit 750d38b. * Removed `https://` from handler list * Removed demo mock * Cleaner errors * Make times and erroring work properly * Print final deployment error * Removed useless promise * Prettified code * Put config utils into correct location * Moved even more config files * Removed useless directory * Removed last useless file * Fixed wrong paths * Fixed unit tests * Update deployment according to handler state, like the server-side loop * More robust deployment mechanism * Poll every 1.5 seconds * Prevent many requests * Show spinner while waiting for deployment to be ready * Render how long the deployment took * Avoid unnecessary repainting * Automatically remove useless `user` property from config when migrating * Fixed property names in `now inspect` * Render platform version for legacy pipeline * Shortened error messages * Support `regions` in the local config * Support for `--regions` added * Add metadata support for legacy deployments (#2) * Share handlers table between deploying and inspecting * Make `now inspect` work nicely * Renamed handlers to builds * Stop sending away description * Bare UI support for builds * Simper logic for rendering builds * Render output of builds * Indicate lambdas in a better way * Render size for build output * Do not show type for version 2 deployments * Fixed time output for `now inspect` * Don't handle BUILDS type * Allow for 100% non-config deployments * Add metadata support for now ls (#3) * Add metadata support for now ls So, we can do things like this: now ls -m key1=value1 -m key2=value2 * Better description * Fix wording * Added final newline * Add sentry (#4) * Revert "Add sentry (#4)" This reverts commit 851d1bd. * Only render build output if it was not copied * Made `now alias` work with latest staging proxy * Revert "Made `now alias` work with latest staging proxy" This reverts commit 16e8998. * Bumped deployments API to the latest version * Made `now rm` work * Do not print `version` warning for single files * Removed useless `fs-extra` dependency * Removed useless dependencies * Default binaries to Node 10 * Bumped Node.js in Circle CI to latest * Bumped Xcode to get latest Node.js for integration tests * Enabled HTTP/2 support * Removed useless code * Added integration test for builds * Bumped `fetch-h2` to the latest version * Avoid performing network request for rendering help * Render note when viewing latest help * Return status code `2` when exiting with help * Fixed test for usage information * Removed wrong text in usage info for Now 2.0 * Removed support for `--links` from v2 pipeline * migrated => upgraded * Added default routing for single files * Make `--token` work as expected * Better message for build errors * Prevent update notification * Prevent update notifications from showing * Only show migration message in debug output * Prevent flickering of state * Improved output * Removed useless assignment * Corrected padding * Less padding before state * Corrected links for global configuration * Fixed integration tests * Render region for Lambdas * Join regions in a better way * Ensure `now.json` and `.nowignore` (the latter worked anyways) are uploaded * Fix `build.env` in new deployment API call (#6) * JSON log the deployment body when debugging (#8) Otherwise it's just `[object Object]` which is useless. * Retry to fetch on error on follow mode (#5) * retry fetch on error on follow mode * improve logging * Fixed `--env` and `--build-env` CLI args (#7) * Fix `--env` and `--build-env` CLI args * Fall back to `undefined` * Ensure `now switch` lists active scope in the beginning * Removed useless code * Error if `env` or `build.env` have wrong types * Made `now inspect` look great * Fixed wrong protocol in URL * Made `now --team` work with users * Leave PHP out of integration test for now * Do not select PHP build * Fixed integration tests * Revert "12.0.0-canary.93" This reverts commit 70a0a59. * Revert "Revert "Add support for top-level "sh" auth"" This reverts commit 4273d62. * Revert "12.0.0-canary.92" This reverts commit 847c71e. * Revert "Add support for top-level "sh" auth" This reverts commit c493d65. * Revert "12.0.0-canary.91" This reverts commit 06c954f. * Revert "Added metadata support for `now inspect` (#1634)" This reverts commit 9567656. * Revert "12.0.0-canary.90" This reverts commit 966737b. * Revert "Added support for deployment metadata (#1604)" This reverts commit 6c1188a.
Ethan-Arrowood
pushed a commit
that referenced
this issue
Jan 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
To avoid dangerous and costly mistakes.
The text was updated successfully, but these errors were encountered: