refactor: Argument parsing (>= node 16)

[aarondill]

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]

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

[aarondill]

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

[fisker]

Let's finish arg parse refactor here? We can merge later https://github.com/keithamus/sort-package-json/pull/283#discussion_r1083528211 .

[aarondill]

Should I edit on top of master or #284?

[fisker]

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

[aarondill]

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

That does seem correct

[aarondill]

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]

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

[fisker]

there are now defaults

Nice! Good to know.

[fisker]

Little help here, made a mistake in #284

https://github.com/keithamus/sort-package-json/blob/07820dbf7af693b3d70aa1ad7d1cfde86cf254af/reporter.js#L63

Should be shouldBeQuiet

[fisker]

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

[aarondill]

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]

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]

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]

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]

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]

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

[aarondill]

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

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

[fisker]

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

[aarondill]

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]

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.

[aarondill]

https://github.com/keithamus/sort-package-json/pull/283#discussion_r1083528211

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