Skip to content
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

Replace cli with commander #892

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Replace cli with commander #892

merged 5 commits into from
Jul 27, 2023

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented May 24, 2023

The current cli.js implementation returns a status code 0 if the command is omitted. This does not seem to be configurable.

$node lib/bin/cli.js --email example@example.com; echo $?
Usage:
  cli.js [OPTIONS] <command> [ARGS]

Options: 
  -u, --email EMAIL      For user create and set password commands, supplies 
                         the email. 
  -h, --help             Display help and usage details

Commands: 
  user-create, user-promote, user-set-password
0

Motivation

This is surprising, and hard to notice if it occurs deep in e.g. a script or docker build.

The original ticket for this is node-js-libs/cli#14, which cites git as an example, but git, like many commands, will print usage and return a non-zero status code:

$ git >/dev/null; echo $?
1
$ grep >/dev/null 2>/dev/null; echo $?
2
$ sed >/dev/null 2>/dev/null; echo $?
1
$ curl >/dev/null 2>/dev/null; echo $?
2
...etc

Other benefits:

  • this removes a dependency, as commander is already included via knex
  • commander is maintained
  • resolves comment: i'm not a huge fan of this library

New implementation:

$ node lib/bin/cli.js --email example@example.com; echo $?
Usage: node lib/bin/cli.js [options] [command]

Options:
  -u, --email <email-address>  
  -h, --help                   display help for command

Commands:
  user-create
  user-promote
  user-set-password
  help [command]               display help for command
1

@alxndrsn alxndrsn marked this pull request as ready for review May 24, 2023 13:29
@alxndrsn alxndrsn changed the title replace cli with commander [DRAFT] Replace cli with commander May 24, 2023
@matthew-white
Copy link
Member

@alxndrsn, just wanted to check about the status of this PR. I was assuming that it's not ready for review because it still has [DRAFT] in its title, but I thought I'd double-check.

@alxndrsn alxndrsn changed the title [DRAFT] Replace cli with commander Replace cli with commander May 31, 2023
@alxndrsn
Copy link
Contributor Author

@matthew-white [DRAFT] removed - this is ready for review 👍

@matthew-white
Copy link
Member

Replacing cli with commander sounds sensible to me if cli isn't maintained and has confusing behavior. I think @ktuite or @sadiqkhoja should weigh in though and do the review.

@ktuite ktuite self-requested a review July 18, 2023 19:52
@alxndrsn alxndrsn merged commit d8615db into getodk:master Jul 27, 2023
1 check passed
@alxndrsn alxndrsn deleted the commander branch July 27, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants