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: Error handling #284

Merged
merged 36 commits into from
Feb 1, 2023
Merged

Conversation

aarondill
Copy link
Contributor

@aarondill aarondill commented Jan 23, 2023

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
Copy link
Contributor Author

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

@keithamus
Copy link
Owner

@aarondill think we should close this one?

@aarondill
Copy link
Contributor Author

@aarondill think we should close this one?

why's that?

@keithamus
Copy link
Owner

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

Stops any successful output. Errors are still outputted.
This commit creates stdout and stderr functions for outputting with
respect to isQuiet and adds support for differing (easier to parse)
messages if not connected to a TTY (eg. a pipe or process substitution)

Additionally changes the 'No matching files.' message to output on stderr
rather than on stdout as it was, which makes sense because this output
represents erroneous usage.
these variables can be set to force output on the specified chanel
as if it were connected to a terminal.
macro.testCLI can now take an argument of the following form:
isTerminal: { stderr: false, stdout: false },
The exit code is now the number of files which failed when sorting.
On a successful run, this will be 0 and exit w/ success, but if any
errors occur, this will be incremented. In check mode, this is fails
+ unsorted.
Wraps fs calls in trycatch to not throw and to count failures. Also better messages and script usage
@fisker
Copy link
Collaborator

fisker commented Jan 30, 2023

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

@fisker
Copy link
Collaborator

fisker commented Jan 30, 2023

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

@aarondill
Copy link
Contributor Author

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
Copy link
Contributor Author

-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
Copy link
Owner

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

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.

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

I'd like to do a refactor.

@aarondill
Copy link
Contributor Author

That seems like a good idea

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

@aarondill c5dd2cc WDYT?

@aarondill
Copy link
Contributor Author

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

@aarondill
Copy link
Contributor Author

aarondill commented Feb 1, 2023

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
Copy link
Collaborator

fisker commented Feb 1, 2023

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
Copy link
Collaborator

fisker commented Feb 1, 2023

Here comes the actual message change. b22d474

Good?

@aarondill
Copy link
Contributor Author

aarondill commented Feb 1, 2023

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
Copy link
Contributor Author

The message change itself looks good

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

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

We can still use util.format.

fisker
fisker previously approved these changes Feb 1, 2023
@fisker fisker changed the title Refactor: Error handling refactor: Error handling Feb 1, 2023
@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

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
Copy link
Contributor Author

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

@fisker
Copy link
Collaborator

fisker commented Feb 1, 2023

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

@keithamus keithamus merged commit 6b1c114 into keithamus:master Feb 1, 2023
@github-actions
Copy link

github-actions bot commented Feb 1, 2023

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aarondill
Copy link
Contributor Author

@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
Copy link
Collaborator

fisker commented Feb 1, 2023

This already merged, you can send a new one.

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

Successfully merging this pull request may close these issues.

3 participants