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

refactor: Argument parsing (>= node 16) #283

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aarondill
Copy link
Contributor

@aarondill aarondill commented Jan 22, 2023

This PR uses node:util.parseArgs for parsing arguments, allowing easy future extendability. The output of the tool remains the same as previous commits but user usage may change. Previous use cases which did not rely on the use of arguments beginning in '-' should remain unchanged, but in this update, arguments which begin with a dash must follow the argument '--', which follows general user expectation. Each of the options(check, quiet, version, and help) have a corresponding short option(c,q,V,h) which can be combined into one (ie -cq = -c -q).

@fisker
Copy link
Collaborator

fisker commented Jan 22, 2023

Why don't we add #282 first and then do a refactor here after.

@aarondill
Copy link
Contributor Author

this should now be up to date with the changes in #282

README.md Outdated Show resolved Hide resolved
cli.js Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

Let's finish arg parse refactor here? We can merge later #283 (comment) .

@aarondill
Copy link
Contributor Author

Should I edit on top of master or #284?

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

#284 don't touch arg parse part, you can start from master branch, arg parsing should only effect L78-L110, isn't it?

@aarondill
Copy link
Contributor Author

arg parsing should only effect L78-L110, isn't it?

That does seem correct

@aarondill
Copy link
Contributor Author

This has been refactored on top of the main branch. The node 12 and 14 tests have both failed, as expected, because parseArgs is not supported in those versions.

@aarondill aarondill changed the title Cli improvments refactor: Argument parsing (>= node 16) Feb 1, 2023
cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
cli.js Outdated Show resolved Hide resolved
This moved argument destructuring to the parseCliArguments function, renames values to options,
renames positionals to patterns, and moves the try catch to the run function
@aarondill
Copy link
Contributor Author

I have moved the try-catch to the run function, and moved destructuring to the parseCliArguments function. Also, there are now defaults(which I previously believed didn't work on node 16), so the boolean conversion is no longer needed.
The parseCliArguments function returns an object with options and patterns. I could separate the options object into further properties, but that would force the seperated definition and assignment of several new variables, which I prefer to avoid

cli.js Outdated Show resolved Hide resolved
@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

there are now defaults

Nice! Good to know.

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Little help here, made a mistake in #284

const { isCheck, isQuiet } = this.#options

Should be shouldBeQuiet

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Very nice! Can you add a test for --unknown-flag?

@aarondill
Copy link
Contributor Author

Is that check in reporter.js on line 73(/63) necessary? We already define the logger functions based on options.shouldBeQuiet on line 18.

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Is that check in reporter.js on line 73(/63) necessary?

It's not, this is why test still pass, but can avoid unnecessary calles.

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Very nice! Can you add a test for --unknown-flag?

Also --no-check (it's common to parse as check: false in other lib)

@aarondill
Copy link
Contributor Author

Very nice! Can you add a test for --unknown-flag?

Also --no-check (it's common to parse as check: false in other lib)

Should this be a global modifier, or only the last counts? ie, should sort-package-json --no-check --check check or not?

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Should this be a global modifier, or only the last counts? ie, should sort-package-json --no-check --check check or not?

Just make a snapshot, not really care how it works.

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Better use --no-version? No one expect it to work.

@aarondill
Copy link
Contributor Author

Better use --no-version? No one expect it to work.

You do want --no-version to be a legal argument?

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

Sorry for not been clear, I didn't expect you to handle --no-check.

@aarondill
Copy link
Contributor Author

Sorry for not been clear, I didn't expect you to handle --no-check.

Oh, so you just want a test that throws? Or do you prefer the new behavior?

@fisker
Copy link
Collaborator

fisker commented Feb 2, 2023

You do want --no-version to be a legal argument?

No, I just want snapshot to show how --no-version works, not care about the result. Same for other --no-*, no need special treatment. Previous version is good.

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

cli.js Show resolved Hide resolved
error.code === 'ERR_PARSE_ARGS_UNKNOWN_OPTION' ||
error.code === 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE'
) {
console.error(`Try 'sort-package-json --help' for more information.`)
Copy link
Owner

@keithamus keithamus Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than telling the user to re-run the command with different arguments, can we just show the help?

Suggested change
console.error(`Try 'sort-package-json --help' for more information.`)
showHelpInformation()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the current mode because it results in shorter output, making it more immediately clear what went wrong, rather than the user having to find the error message above the wall of text. If you are insistent that we should show the help menu, perhaps we move the error displaying to after the help, so the user can see what went wrong without scrolling

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about show it directly too, but I agree error message is more important to tell user what's wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithamus, thoughts?

Copy link
Owner

@keithamus keithamus Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy displaying an error message but I think it's important to show the help. Perhaps a more explicit error of --foo is not a valid argument would also be better here?

To clarify - IMO we should show both help and an error telling them why they're seeing the help instead of the command running.

Copy link
Contributor Author

@aarondill aarondill Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the util.parseArgs function doesn't seem to expose a way to determine the invalid argument which was passed, other than parsing error.message; however error.message has no guarantee of consistency between versions (Errors | Node.js).

Regarding displaying help, would you suggest the following, or the error at the end? I feel that the help might be slightly overwhelming when just searching for the error.

$ sort-package-json --not-an-option
Unknown option '--not-an-option'. To specify a positional argument starting with a '-',
place it at the end of the command after '--', as in '-- "--not-an-option"
Usage: sort-package-json [options] [file/glob ...]

Sort package.json files.
If file/glob is omitted, './package.json' file will be processed.

  -c, --check   Check if files are sorted
  -q, --quiet   Don't output success messages
  -h, --help    Display this help
  -v, --version Display the package version

I personally prefer the shorter output (see below), as it directly tells the user what went wrong, without having to find it, as well as telling them how to find help if they aren't sure already how to fix their error.

$ sort-package-json --not-an-option
Unknown option '--not-an-option'. To specify a positional argument starting with a '-',
place it at the end of the command after '--', as in '-- "--not-an-option"
Try 'sort-package-json --help' for more information.

Additionally, this is the format followed by many default tools, such as ls, df, du, and diff:

$ df --not-an-option
df: unrecognized option '--not-an-option'
Try 'df --help' for more information.
$ ls --not-an-option
ls: unrecognized option '--not-an-option'
Try 'ls --help' for more information.
$ du --not-an-option
du: unrecognized option '--not-an-option'
Try 'du --help' for more information.
$ diff --not-an-option
diff: unrecognized option '--not-an-option'
diff: Try 'diff --help' for more information.

The other reasonable possibility is to display an abbreviated usage menu, such as the one displayed by git (below), however this forces the maintenance of both help menus when the arguments or usage of the cli changes.

$ git --not-an-option
unknown option: --not-an-option
usage: git [--version] [--help] [-C <path>] [-c <name>=<value>]
           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]
           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
           [--super-prefix=<path>] [--config-env=<name>=<envvar>]
           <command> [<args>]

@fisker fisker mentioned this pull request Apr 19, 2023
@aarondill
Copy link
Contributor Author

aarondill commented May 22, 2023

#283 (comment)

are we ready to start looking at merging this PR again, because the node 12 EOL was reached, or are we still supporting V12?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants