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

feat: set exit code depending on what went wrong #66

Merged
merged 9 commits into from
Oct 28, 2022
Merged

Conversation

ericcornelissen
Copy link
Collaborator

@ericcornelissen ericcornelissen commented Oct 20, 2022

Update the CLI to exit with unique exit codes depending on what went wrong:

  • 0: Success
  • 1: There are linting errors
  • 2: Something unexpected went wrong
  • 3: Interrupted
  • 4: Configuration-related problem

Suggestions are welcome.

@mondeja
Copy link
Member

mondeja commented Oct 21, 2022

What about:

  • 3 configuration file passed to --config not found
  • 4 no configuration files discovered
  • 5 configuration file invalid
  • 6 linting interrupted by the user (catching signal sent by Ctrl + C)
  • 7 command line usage error

@ericcornelissen
Copy link
Collaborator Author

Thanks for the feedback @mondeja 🙂

  • 3 configuration file passed to --config not found
  • 4 no configuration files discovered
  • 5 configuration file invalid

I'm not sure the distinction between 3 and 4 makes sense, as you state in #64:

If a file is passed in the --config CLI argument, it will be loaded or exits with code 1

So, if --config is provided and the exit code is 3 it should be clear that that configuration file isn't found, whereas if no --config is provided and the exit code is 3 it should be clear no configuration file was found.

Additionally, I was actually wondering if we should have just one exit code for when something's wrong related to the configuration - the log output should tell you what exactly went wrong. Even with 2 exit codes related to configuration I wondered if I was being too granular, with 3 even more so, and I'm afraid we could go more granular (maybe after new features are added). What do you think?

  • 6 linting interrupted by the user (catching signal sent by Ctrl + C)

This makes some sense to me, but I'm not sure how common and/or useful this is. Do other CLI applications do this? Could you provide some examples?

If we want to add this, this snippet can be used as a starting point (ref):

process.on('SIGINT', () => {
  // Handle `Ctrl`+`C`
});
  • 7 command line usage error

Can you elaborate? Do you mean that this exit code should be used if, with the provided options, the CLI can't run.

If yes, I'm not sure if that's currently applicable 🤔 Where in the code would we exit in this manner?

Use the "projects" directory to create a broken project with an invalid
configuration file.
Invalid could mean that the configuration is invalid for SVGLint, but
it's supposed to mean that it's invalid JavaScript. Broken seems clearer
while being equally brief.
@mondeja
Copy link
Member

mondeja commented Oct 23, 2022

From the perspective of a writer of scripts, always is easy and convenient to check the exit code rather than outputs to know what is really happening.

So, if --config is provided and the exit code is 3 it should be clear that that configuration file isn't found, whereas if no --config is provided and the exit code is 3 it should be clear no configuration file was found.

I'm thinking in the typical context where an argument is built dinamically with Bash, Shell, Powershell... You don't know if the argument is passed until is executed.

Additionally, I was actually wondering if we should have just one exit code for when something's wrong related to the configuration - the log output should tell you what exactly went wrong. Even with 2 exit codes related to configuration I wondered if I was being too granular, with 3 even more so, and I'm afraid we could go more granular (maybe after new features are added). What do you think?

As you prefer, I don't have a strong opinion about it.

This makes some sense to me, but I'm not sure how common and/or useful this is. Do other CLI applications do this? Could you provide some examples?

See pytest exit codes.

Can you elaborate? Do you mean that this exit code should be used if, with the provided options, the CLI can't run.

For example, if I pass an argument that do not exist like --foo. That would be a command line usage error.

- Combine exit codes related to configuration into one
- Add exit code for SIGINT
- Define `EXIT_CODES` object for readability and to avoid duplication
@ericcornelissen
Copy link
Collaborator Author

From the perspective of a writer of scripts, always is easy and convenient to check the exit code rather than outputs to know what is really happening.

That actually makes a lot of sense as a guiding principle, thanks for sharing.

Additionally, [...]

As you prefer, I don't have a strong opinion about it.

In that case I'm going to simplify it for now and stick with one exit code to represent configuration problems.

This makes some sense to me, but I'm not sure how common and/or useful this is. Do other CLI applications do this? Could you provide some examples?

See pytest exit codes.

Thanks for the example, in that case I'm okay adding it 👍

Can you elaborate? Do you mean that this exit code should be used if, with the provided options, the CLI can't run.

For example, if I pass an argument that do not exist like --foo. That would be a command line usage error.

I see, thanks for elaborating. However, since using non-existent arguments isn't currently a problem I won't change that (at least within the scope of this Pull Request).

@ericcornelissen ericcornelissen changed the title Set exit code depending on what went wrong feat: set exit code depending on what went wrong Oct 28, 2022
@ericcornelissen ericcornelissen merged commit 65f16bd into master Oct 28, 2022
@ericcornelissen ericcornelissen deleted the exit-codes branch October 28, 2022 15:00
@github-actions
Copy link

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants