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

fix: Add --no-exit-code option to prevent spelling issues from impacting the exit code. #4809

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

fughilli
Copy link
Contributor

@fughilli fughilli commented Sep 8, 2023

This PR implements a simple --allow-failure flag, which configures cspell to return 0 exit code even when there are findings. This is useful for uses in CI (such as in a pre-commit hook) to allow reporting findings without failing the pre-commit.

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 9, 2023

@fughilli,

Thank you. I'm open to the idea. I have to think about the name.

Things to consider:

There is a difference between issues and errors.

  • Spelling Issues - something was detected in a document.
    • ignored - the word has been marked to be ignored.
    • suggest - there is a suggested change, but it is not considered incorrect.
    • unknown - the word was not found in any dictionary and is considered incorrect.
    • flagged - the word has been flagged as forbidden.

Only unknown and flagged issues are reported and will cause an exit code of 1.

  • Errors - something went wrong.
    • file errors
    • bad or missing configuration.

This new option should only impact issues and not errors.

Errors can be handled using the || command line syntax: cspell . || echo ok.

Ideas:

  • --exit-code-for-issues <number> - If issues (unknown or flagged) are found, use the <number> given.

A future option: --issues-level <suggest|unknown|flagged> - to set the level where the exit code is used.

Copy link
Collaborator

@Jason3S Jason3S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #4809 (comment)

How about --no-exit-code - Spelling issues will not set the exit code.? Implies --no-must-find-files.

@@ -170,7 +171,8 @@ export function commandLint(prog: Command): Command {
throw new CheckFailed('outputHelp', 1);
}
if (result.issues || result.errors || (mustFindFiles && !result.files)) {
throw new CheckFailed('check failed', 1);
const exitCode = options.allowFailure ? 0 : 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result.errors is a real error.

Logic:

Suggested change
const exitCode = options.allowFailure ? 0 : 1;
const exitCode = result.errors || !options.allowFailure ? 1 : 0;

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 9, 2023

@fughilli,

It will be necessary to update the snapshots.

In packages/cspell:

pnpm vitest run -u

@fughilli
Copy link
Contributor Author

fughilli commented Sep 11, 2023

I'm open to either --exit-code-for-issues or --no-exit-code, which would be preferable? Good catch on the result.errors nuance, I wasn't paying close enough attention to the conditional expression.

Also, I was trying to build using pnpm build and ran into:

...
packages/cspell-lib build$ tsc -b . && pnpm run build:api
packages/cspell-lib build: error TS6305: Output file '/Users/kbalke/Projects/git_checkouts/vendored/cspell/packages/cspell-lib/src/lib-cjs/pkg-info.d.cts' has not been built from source file '/Users/kbalke/Projects
/git_checkouts/vendored/cspell/packages/cspell-lib/src/lib-cjs/pkg-info.cts'.
packages/cspell-lib build:   The file is in the program because:
packages/cspell-lib build:     Matched by default include pattern '**/*'
packages/cspell-lib build: Failed
/Users/kbalke/Projects/git_checkouts/vendored/cspell/packages/cspell-lib:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  cspell-lib@7.3.2 build: `tsc -b . && pnpm run build:api`
Exit status 1
 ELIFECYCLE  Command failed with exit code 1.

Any ideas?

@Jason3S
Copy link
Collaborator

Jason3S commented Sep 11, 2023

@fughilli,

Let's go with --no-exit-code, I think that is the simplest for users to understand.

As far as the build, I think you might need to clean first: pnpm i && pnpm clean at the root.

@fughilli
Copy link
Contributor Author

How does this look? @Jason3S

The `--no-exit-code` option becomes `exitCode`.
@Jason3S
Copy link
Collaborator

Jason3S commented Sep 12, 2023

@fughilli,

The command line parser turns --no-exit-code into exitCode.

I corrects the Options interface and updated the snapshots.

@Jason3S Jason3S changed the title Allow failure fix: Add --no-exit-code option to prevent spelling issues from impacting the exit code. Sep 12, 2023
@Jason3S Jason3S merged commit 6df63fb into streetsidesoftware:main Sep 12, 2023
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants