pzavolinsky / ts-unused-exports

ts-unused-exports finds unused exported symbols in your Typescript project
MIT License
749 stars 49 forks source link

Feature request: exit with non-zero status when unused exports are found by default #60

Closed emlai closed 5 years ago

emlai commented 5 years ago

I'm trying to use ts-unused-exports in a CI pipeline so I want it to return a non-zero exit status if it finds any issues (similar to eslint etc.). Currently (v3.0.0) I have to use --exitWithCount which is a bit annoying, especially since I don't care about the count, just whether any issues were found.

My suggestion is to change the default behavior to return 1 if any unused exports are found, and 0 otherwise.

mrseanryan commented 5 years ago

Hi @emlai

This is an interesting issue, since other people want the default to be exit code = 0 :) .

(ref https://github.com/pzavolinsky/ts-unused-exports/issues/35)

I think a non-zero exit code usually means something bad happened - the process failed to complete - and it's not clear that finding some unused exports comes under that category.

Also, for some projects, there could always be some unused exports (for example a big project where there is not time fix everything...) and then there would always be a non zero exit code.

I think non zero means the process failed to complete fully (ref https://www.tldp.org/LDP/abs/html/exit-status.html) and would cause most scripts to fail.

An alternative to provide another command - like ./bin/ts-unused-exports-exit-with-count

But it seems that convention would agree with the current behaviour...

@pzavolinsky what do you think?

emlai commented 5 years ago

I'm not sure if #35 was about returning the number of files as the exit code, or about returning a non-zero exit code altogether. (For the record, I agree returning the number of files is not a good idea.)

IMO the usual conventions should be followed in this case. This is a lint-like tool (and might be integrated as a rule within existing linters in the future), and both eslint and tslint exit with non-zero if linting errors are found:

eslint:

When linting files, ESLint will exit with one of the following exit codes:

0: Linting was successful and there are no linting errors. If the --max-warnings flag is set to n, the number of linting warnings is at most n. 1: Linting was successful and there is at least one linting error, or there are more linting warnings than allowed by the --max-warnings option. 2: Linting was unsuccessful due to a configuration problem or an internal error.

tslint:

The CLI process may exit with the following codes:

0: Linting succeeded without errors (warnings may have occurred) 1: An invalid command line argument or combination thereof was used 2: Linting failed with one or more rule violations with severity error

So eslint and tslint have the concept of "warnings" for things you don't want to affect the exit status. Not sure if a "warning" mode makes sense for ts-unused-exports.

One more thought: it's easy to ignore a non-zero exit status if you don't care about it, like so:

ts-unused-exports || true

Of course the above might cause configuration errors and internal errors to go unnoticed.

mrseanryan commented 5 years ago

Thank you @emlai for the info about eslint/tslint.

Now that I see it, indeed the eslint approach sounds good - it seems more suitable than the tslint approach, as we don't have a concept of severity.

pzavolinsky commented 5 years ago

I think we should stick with eslint's.

If someone really really needs the exit status to be always 0 regardless of anything they can always do ts-unused exports .... || true.

I think we can safely break current exit status behavior in order to be more consistent. I'll happily bump the major version for this.

mrseanryan commented 5 years ago

@emlai this is done - released with version 4.

thank you for raising it.