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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
54270fd
feat: use util.parseArgs
aarondill Feb 1, 2023
31d488a
refactor: destrucure argument in parseCLIArguments
aarondill Feb 2, 2023
0d12907
refactor: move default pattern to parseCliArgument
aarondill Feb 2, 2023
c183b60
fix: typo isQuiet->shouldBeQuiet
aarondill Feb 2, 2023
834ea29
fix: show help on illegal option value
aarondill Feb 2, 2023
aaa38d1
test: added tests for illegal argument usage
aarondill Feb 2, 2023
10a6067
feat: add support for --no-check and --no-quiet
aarondill Feb 2, 2023
65b60d1
Revert "feat: add support for --no-check and --no-quiet"
aarondill Feb 2, 2023
c3f5a6b
test: add tests which throw for --no-* arguments
aarondill Feb 2, 2023
baaa560
test: update tests
fisker Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env node
import { globbySync } from 'globby'
import fs from 'node:fs'
import { parseArgs } from 'node:util'
aarondill marked this conversation as resolved.
Show resolved Hide resolved
import sortPackageJson from './index.js'
import Reporter from './reporter.js'

Expand All @@ -27,6 +28,20 @@ If file/glob is omitted, './package.json' file will be processed.
)
}

function parseCliArguments() {
const { values: options, positionals: patterns } = parseArgs({
options: {
check: { type: 'boolean', short: 'c', default: false },
quiet: { type: 'boolean', short: 'q', default: false },
version: { type: 'boolean', short: 'v', default: false },
help: { type: 'boolean', short: 'h', default: false },
keithamus marked this conversation as resolved.
Show resolved Hide resolved
},
allowPositionals: true,
strict: true,
})
return { options, patterns }
}

function sortPackageJsonFile(file, reporter, isCheck) {
const original = fs.readFileSync(file, 'utf8')
const sorted = sortPackageJson(original)
Expand Down Expand Up @@ -58,41 +73,34 @@ function sortPackageJsonFiles(patterns, options) {
}

function run() {
const cliArguments = process.argv.slice(2)
let options, patterns
try {
;({ options, patterns } = parseCliArguments())
} catch (error) {
process.exitCode = 2
console.error(error.message)
if (error.code === 'ERR_PARSE_ARGS_UNKNOWN_OPTION') {
console.error(`Try 'sort-package-json --help' for more information.`)
Copy link
Owner

@keithamus keithamus Feb 2, 2023 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

}
return
}

if (
cliArguments.some((argument) => argument === '--help' || argument === '-h')
) {
if (options.help) {
return showHelpInformation()
}

if (
cliArguments.some(
(argument) => argument === '--version' || argument === '-v',
)
) {
if (options.version) {
return showVersion()
}

const patterns = []
let isCheck = false
let shouldBeQuiet = false

for (const argument of cliArguments) {
if (argument === '--check' || argument === '-c') {
isCheck = true
} else if (argument === '--quiet' || argument === '-q') {
shouldBeQuiet = true
} else {
patterns.push(argument)
}
}

if (!patterns.length) {
aarondill marked this conversation as resolved.
Show resolved Hide resolved
patterns[0] = 'package.json'
}

sortPackageJsonFiles(patterns, { isCheck, shouldBeQuiet })
sortPackageJsonFiles(patterns, {
isCheck: options.check,
shouldBeQuiet: options.quiet,
})
}

run()