refactor: Error handling

[aarondill]

This PR refactors the code to enclose common error points in try catch blocks and outputting the message property on the error to the user on stderr. This further allows tolerant errors where the program is not halted, but instead continues to process other files.

[aarondill]

This and #283 will go well together, however this commit only relies on #281 before it can be merged

[keithamus]

@aarondill think we should close this one?

[aarondill]

@aarondill think we should close this one?

why's that?

[keithamus]

Perhaps it needs to be rebased/refactored. I'm struggling to see what it adds on top of #281

[fisker]

👎 For increasing complex. Can't get why we need these changes.

[fisker]

Even if we want a better error message, we can just try/catch those possible errors and print a friendly message.

[aarondill]

I have rebased this onto keithamus/master and removed extra changes to make the diff slightly clearer. The history is a bit of a mess, I could clean it up if you'd like, but I figure it'll get squashed in the merge anyways.

This builds on 281 by improving the developer experience when using the CLI in shell scripts. You can read the changes in the new section of the readme, but it boils down to this:

• Wrap common failure points in try catch and report errors without failing
• Fully utilize stdout and stderr. This, for example, allows for sort-package-json **/package.json 2>/dev/null to output only success
• Change the output depending on tty presence. This allows the user all the information, but gives only important information to scripts utilizing this, allowing for easier parsing.
  • If there is not a tty
    • stderr contains only files which failed in either mode: sort-package-json **/package.json 2>&1 1>/dev/null | look-for-certain-failed-files
    • stdout contains files which were successful, in check mode, these are files which are already sorted.
  • With a tty
    • stderr contains files which failed, and the Error.message which came with them
    • stdout contains the files which were successful, with human readable messages (ie package.json was sorted!)

[aarondill]

-1 For increasing complex. Can't get why we need these changes.

This does increase complexity, but it allows for much easier scripting, without difficult parsing. which is especially difficult if all messages are outputted to stdout. Even if we decide to simplify to the extreme, we should still at least ensure that expected errors don't halt the program and that error messages are outputted to stderr rather than stdout.

[keithamus]

Don't mind untidy history, as long as the diff+description makes up for it :wink:.

I see where you're going with this, and I don't mind improving the output for scripts, but I think we can perhaps improve error messaging in general. Let's first get better error messages in the general case, then let's work to figure out a way to make error messages more parseable. TTY detection is a bit too much magic for my preference.

[aarondill]

All TTY detection has been removed, keeping only the stdout/stderr distinction and the nicer error handling and display.

[fisker]

I'd like to review, but later.

[fisker]

I think we should print a summary to show how many files errored, just like how many files not sorted.

[aarondill]

I think we should print a summary to show how many files errored, just like how many files not sorted.

A template/idea for this would be greatly appreciated. I am thinking something like this:

`Found ${files.length} files
${errorFiles} files could not be checked.
${notSorted} files were not sorted
${files.length-notSorted} files were already sorted`

Other ideas are appreciated.

[fisker]

How about

isCheck: true

Found ${files.length} files.
${status.failed} files could not be checked. // only if errored
${status.notSorted} files were not sorted.
${status.notChanged} files were already sorted.`

isCheck: false

Found ${files.length} files
${status.failed} files could not be sorted. // only if errored
${status.succeed} files successfully sorted.
${status.notChanged} files were already sorted.`

We may want show All files sorted. if all succeed.

[aarondill]

A summary has been added to the end of the running (if not isQuiet), additionally, I wrapped the fs.writeFileSync call in a try-catch, as it seems to have been missed the first time around.

[aarondill]

Some example runs from within this repo

./cli.js --check **/package.json:

Found 1220 files.
1 files could not be checked.
1193 files were not sorted.
26 files were already sorted.

./cli.js **/package.json:

Found 1220 files.
1 files could not be sorted.
1193 files successfully sorted.
26 files were already sorted.

[fisker]

I'd like to do a refactor.

[aarondill]

That seems like a good idea

[fisker]

@aarondill https://github.com/keithamus/sort-package-json/pull/284/commits/c5dd2cc715aaec722a710f566b8f6ad6cf1a0055 WDYT?

[aarondill]

It looks good. To clarify, this doesn’t affect any of the output, correct? I didn’t see any changes in the snapshot

[aarondill]

also, this seems slightly overly verbose:

      isCheck
        ? `${status.failedFilesCount} files could not be checked.`
        : `${status.failedFilesCount} files could not be sorted.`,

perhaps this could be simplified to

`${status.failedFilesCount} files could not be ${isCheck ? 'checked' : 'sorted'}.`

potentially on the next line as well.

This is entirely stylistic though, and may be better in its current state if we ever plan to change these output statements

[fisker]

To clarify, this doesn’t affect any of the output, correct?

Not changed.

This is entirely stylistic though, and may be better in its current state if we ever plan to change these output statements

That's why I wrote like this.

[fisker]

Here comes the actual message change. https://github.com/keithamus/sort-package-json/pull/284/commits/b22d474c8095a2083a0e6dd7cb4f1a4e5d20c6c1

Good?

[aarondill]

Perhaps use template strings, rather than the console.log format strings(%s), in case our implementation of the log function changes in the future

[aarondill]

The message change itself looks good

[fisker]

in case our implementation of the log function changes in the future

We can still use util.format.

[fisker]

I extracted reporter to a separate file, you can just add changes to that file if you still want do the TTY thing, but I suggest we should keep things simple.

[aarondill]

For now, lets keep it simple, especially since @keithamus has said they prefer it that way

[fisker]

@aarondill We need update https://github.com/keithamus/sort-package-json#usage, since output changed.

[github-actions[bot]]

:tada: This PR is included in version 2.4.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[aarondill]

@aarondill We need update https://github.com/keithamus/sort-package-json#usage, since output changed.

I notice that you merged this PR. would you like me to create a new PR or just continue to commit this change here?

[fisker]

This already merged, you can send a new one.