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

aarondill · 2023-01-22T05:36:27Z

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 · 2023-01-22T12:34:41Z

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

aarondill · 2023-01-22T18:10:24Z

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

README.md

cli.js

fisker · 2023-02-01T04:37:27Z

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

aarondill · 2023-02-01T04:50:06Z

Should I edit on top of master or #284?

fisker · 2023-02-01T05:43:17Z

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

aarondill · 2023-02-01T05:56:14Z

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

That does seem correct

aarondill · 2023-02-01T17:46:26Z

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.

cli.js

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 · 2023-02-02T04:03:55Z

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

fisker · 2023-02-02T04:45:37Z

there are now defaults

Nice! Good to know.

fisker · 2023-02-02T04:46:46Z

Little help here, made a mistake in #284

sort-package-json/reporter.js

Line 63 in 07820db

const { isCheck, isQuiet } = this.#options

Should be shouldBeQuiet

fisker · 2023-02-02T04:49:47Z

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

aarondill · 2023-02-02T04:51:43Z

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 · 2023-02-02T04:53:06Z

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 · 2023-02-02T05:02:05Z

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 · 2023-02-02T05:11:55Z

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 · 2023-02-02T05:14:12Z

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 · 2023-02-02T05:16:21Z

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

aarondill · 2023-02-02T05:18:35Z

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

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

fisker · 2023-02-02T05:18:56Z

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

aarondill · 2023-02-02T05:20:12Z

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 · 2023-02-02T05:21:24Z

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.

This reverts commit 10a6067.

fisker

Great work!

cli.js

keithamus · 2023-02-02T10:34:06Z

cli.js

+      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.`)


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()

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

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

@keithamus, thoughts?

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.

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>]

aarondill · 2023-05-22T00:56:20Z

#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?

aarondill mentioned this pull request Jan 22, 2023

feat(cli): add --quiet / -q option #281

Merged

aarondill mentioned this pull request Jan 22, 2023

feat(cli): added --version and --help options #282

Merged

fisker reviewed Jan 22, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

fisker reviewed Jan 22, 2023

View reviewed changes

cli.js Show resolved Hide resolved

fisker reviewed Jan 22, 2023

View reviewed changes

cli.js Outdated Show resolved Hide resolved

aarondill mentioned this pull request Jan 23, 2023

refactor: Error handling #284

Merged

feat: use util.parseArgs

54270fd

aarondill force-pushed the cli_improvments branch from b0581a6 to 54270fd Compare February 1, 2023 17:43

aarondill changed the title ~~Cli improvments~~ refactor: Argument parsing (>= node 16) Feb 1, 2023

fisker reviewed Feb 1, 2023

View reviewed changes

cli.js Outdated Show resolved Hide resolved

cli.js Outdated Show resolved Hide resolved

cli.js Outdated Show resolved Hide resolved

refactor: destrucure argument in parseCLIArguments

31d488a

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

fisker reviewed Feb 2, 2023

View reviewed changes

cli.js Outdated Show resolved Hide resolved

refactor: move default pattern to parseCliArgument

0d12907

fix: typo isQuiet->shouldBeQuiet

c183b60

aarondill added 2 commits February 1, 2023 23:04

fix: show help on illegal option value

834ea29

test: added tests for illegal argument usage

aaa38d1

feat: add support for --no-check and --no-quiet

10a6067

aarondill and others added 3 commits February 1, 2023 23:23

Revert "feat: add support for --no-check and --no-quiet"

65b60d1

This reverts commit 10a6067.

test: add tests which throw for --no-* arguments

c3f5a6b

test: update tests

baaa560

fisker approved these changes Feb 2, 2023

View reviewed changes

fisker reviewed Feb 2, 2023

View reviewed changes

cli.js Show resolved Hide resolved

keithamus reviewed Feb 2, 2023

View reviewed changes

fisker mentioned this pull request Apr 19, 2023

feat: add --stdin flag #296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aarondill commented Jan 22, 2023 •

edited

Loading

fisker commented Jan 22, 2023

aarondill commented Jan 22, 2023

fisker commented Feb 1, 2023 •

edited

Loading

aarondill commented Feb 1, 2023

fisker commented Feb 1, 2023 •

edited

Loading

aarondill commented Feb 1, 2023

aarondill commented Feb 1, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023 •

edited

Loading

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker left a comment

keithamus Feb 2, 2023 •

edited

Loading

aarondill Feb 2, 2023

fisker Feb 2, 2023

aarondill Feb 2, 2023

keithamus Feb 2, 2023 •

edited

Loading

aarondill Feb 2, 2023 •

edited

Loading

aarondill commented May 22, 2023 •

edited

Loading

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

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

Are you sure you want to change the base?

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

Conversation

aarondill commented Jan 22, 2023 • edited Loading

fisker commented Jan 22, 2023

aarondill commented Jan 22, 2023

fisker commented Feb 1, 2023 • edited Loading

aarondill commented Feb 1, 2023

fisker commented Feb 1, 2023 • edited Loading

aarondill commented Feb 1, 2023

aarondill commented Feb 1, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023 • edited Loading

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

aarondill commented Feb 2, 2023

fisker commented Feb 2, 2023

fisker left a comment

Choose a reason for hiding this comment

keithamus Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

aarondill Feb 2, 2023

Choose a reason for hiding this comment

fisker Feb 2, 2023

Choose a reason for hiding this comment

aarondill Feb 2, 2023

Choose a reason for hiding this comment

keithamus Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

aarondill Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

aarondill commented May 22, 2023 • edited Loading

aarondill commented Jan 22, 2023 •

edited

Loading

fisker commented Feb 1, 2023 •

edited

Loading

fisker commented Feb 1, 2023 •

edited

Loading

fisker commented Feb 2, 2023 •

edited

Loading

keithamus Feb 2, 2023 •

edited

Loading

keithamus Feb 2, 2023 •

edited

Loading

aarondill Feb 2, 2023 •

edited

Loading

aarondill commented May 22, 2023 •

edited

Loading