From 65e49e31bcf3541c49ccd885baa5d6d8d80539c7 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Tue, 12 Jan 2021 12:44:31 +0200 Subject: [PATCH 1/9] Added rfc --- .../2021-break-on-parsing-errors/README.md | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 designs/2021-break-on-parsing-errors/README.md diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md new file mode 100644 index 00000000..1d89f01a --- /dev/null +++ b/designs/2021-break-on-parsing-errors/README.md @@ -0,0 +1,49 @@ +- Repo: eslint/eslint +- Start Date: 2021/1/12 +- RFC PR: +- Authors: [A-Katopodis](https://github.com/A-Katopodis) + +# (Break on parsing errors) + +## Summary + +The suggest change is ESLint support a parameter that will break the ESLint run when it finds configuration errors. + +## Motivation + +We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the command would report all errors in each file and also exit with a successfull code. + + +## Detailed Design +Adding a new option in ESLint called `--break-on-error` which will be which will report and nonzero exit code on case of parsing errors. The most current similar option is `--max-warning`. + +A command example: + +`eslint **.js --break-on-error` + +If this command finds any kind of file which may use ECMAScript modules it will report a non-zero exit code (since the default for `sourceType` is script.) + +Without `--break-on-error` (the current behavior) a user of ESLint would have to look at each and every result to distinquish if there were any kind of missconfiguration/parsing errors as having a normal error in a file is reported the same way as a parsing error. This validation becomes harder for CI pipelines who want to ensure that ESLint reports any rules correctly and was run sucessfully. + +The expected behavior of the command should gather be able to read the reported errors that eslint reports. If at least one missconfiguration is found it exits with a non-zero exit code, preferably 2 so we can distinguish it from the exit code 1. + +## Documentation +It may be a good idea on why we are introducing a option that changes the way ESLint behaves. Will leave the choice to the ESLint team. + + +## Drawbacks +Some users who may want to enable this new feature may find themselves needing to reconfigure their process. It can change some CI pipelines if used. But since its optional the impact is minimal and it may allow them to discover issues. + + +## Backwards Compatibility Analysis +There are no expected backward compatibility issues. The parameter will be disabled by default and only users who will use this new parameter will experience any kind of change. + + +## Open Questions +Do we want to still report all errors or only failed linting errors when `break-on-lint-error` is passed? + +Assume that for some files ESLint successfully lints them and reports a rule but for some others it doesnt. +If ESLint finds a single file that has a parsing error should it report just that file or every rule as well? + +## Related Discussions +https://github.com/eslint/eslint/issues/13711 From 4f12bdc3e37b47dd7ec20ef3b216510bb876c0ea Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Tue, 12 Jan 2021 12:55:24 +0200 Subject: [PATCH 2/9] Updated rfc --- designs/2021-break-on-parsing-errors/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 1d89f01a..24a53102 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -1,13 +1,13 @@ - Repo: eslint/eslint - Start Date: 2021/1/12 -- RFC PR: +- RFC PR: https://github.com/eslint/rfcs/pull/75 - Authors: [A-Katopodis](https://github.com/A-Katopodis) # (Break on parsing errors) ## Summary -The suggest change is ESLint support a parameter that will break the ESLint run when it finds configuration errors. +The suggest change is ESLint support a parameter that will break the ESLint run when it finds configuration errors. The option will be an opt-in argument called `--break-on-error`. ## Motivation From 7a1426d8fde269d5c69c97a8f0623f38747a3af9 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Tue, 12 Jan 2021 13:01:08 +0200 Subject: [PATCH 3/9] New: Break on parsing errors --- designs/2021-break-on-parsing-errors/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 24a53102..f935a85c 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -43,7 +43,8 @@ There are no expected backward compatibility issues. The parameter will be disab Do we want to still report all errors or only failed linting errors when `break-on-lint-error` is passed? Assume that for some files ESLint successfully lints them and reports a rule but for some others it doesnt. -If ESLint finds a single file that has a parsing error should it report just that file or every rule as well? +If ESLint finds a single file that has a parsing error should it report just that file or every rule as +well? ## Related Discussions https://github.com/eslint/eslint/issues/13711 From 51cd72f1206d0b7b687524d51deb0564a8792583 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Tue, 12 Jan 2021 19:12:31 +0200 Subject: [PATCH 4/9] Updated rfc --- designs/2021-break-on-parsing-errors/README.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index f935a85c..4a704ebc 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -11,8 +11,15 @@ The suggest change is ESLint support a parameter that will break the ESLint run ## Motivation -We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the command would report all errors in each file and also exit with a successfull code. +We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the command would report all errors in each file and also exit with a 1 exit code. +According to the eslint docs about 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. + +Being able to exit with code `2` instead of `1` it will allow for CI pipelines to better understand the results. ## Detailed Design Adding a new option in ESLint called `--break-on-error` which will be which will report and nonzero exit code on case of parsing errors. The most current similar option is `--max-warning`. @@ -25,7 +32,7 @@ If this command finds any kind of file which may use ECMAScript modules it will Without `--break-on-error` (the current behavior) a user of ESLint would have to look at each and every result to distinquish if there were any kind of missconfiguration/parsing errors as having a normal error in a file is reported the same way as a parsing error. This validation becomes harder for CI pipelines who want to ensure that ESLint reports any rules correctly and was run sucessfully. -The expected behavior of the command should gather be able to read the reported errors that eslint reports. If at least one missconfiguration is found it exits with a non-zero exit code, preferably 2 so we can distinguish it from the exit code 1. +The expected behavior of the command should gather be able to read the reported errors that eslint reports. If at least one missconfiguration is found it exits with a non-zero exit code, 2 so we can distinguish it from the exit code 1. ## Documentation It may be a good idea on why we are introducing a option that changes the way ESLint behaves. Will leave the choice to the ESLint team. From bbb0f84989637561ef303980f2a7dc789043ac16 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Fri, 22 Jan 2021 17:07:01 +0200 Subject: [PATCH 5/9] Updated RFC for PR feedback --- .../2021-break-on-parsing-errors/README.md | 150 ++++++++++++++++-- 1 file changed, 139 insertions(+), 11 deletions(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 4a704ebc..86b3e123 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -7,11 +7,11 @@ ## Summary -The suggest change is ESLint support a parameter that will break the ESLint run when it finds configuration errors. The option will be an opt-in argument called `--break-on-error`. +The suggested change is for ESLint tp support an argument that will make the CLI exit with code 2 if any fatal errors are reported. The option will be an opt-in boolean argument called `--fatal-parse-error`. ## Motivation -We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the command would report all errors in each file and also exit with a 1 exit code. +We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the error would be a `fatal` one but the exit code would be `1`. There was no distiction between a run with no fatal errors and with fatal errors. According to the eslint docs about exit codes: @@ -19,23 +19,151 @@ According to the eslint docs about exit codes: - 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. -Being able to exit with code `2` instead of `1` it will allow for CI pipelines to better understand the results. +Being able to exit with code `2` instead of `1` it will allow for some CI pipelines to better understand the results and react differently if ESLint reports any `fatal` errors. -## Detailed Design -Adding a new option in ESLint called `--break-on-error` which will be which will report and nonzero exit code on case of parsing errors. The most current similar option is `--max-warning`. +Altough it is possible to filter out the results it will not allow the user -A command example: +## Detailed Design -`eslint **.js --break-on-error` +Design Summary: -If this command finds any kind of file which may use ECMAScript modules it will report a non-zero exit code (since the default for `sourceType` is script.) +1. Add a `fatal-parse-error` option. +2. Gather and report the fatal messages from `cli-engine`. +3. Return exit code `2` in the case of any fatal errors on `cli` -Without `--break-on-error` (the current behavior) a user of ESLint would have to look at each and every result to distinquish if there were any kind of missconfiguration/parsing errors as having a normal error in a file is reported the same way as a parsing error. This validation becomes harder for CI pipelines who want to ensure that ESLint reports any rules correctly and was run sucessfully. +A command example: -The expected behavior of the command should gather be able to read the reported errors that eslint reports. If at least one missconfiguration is found it exits with a non-zero exit code, 2 so we can distinguish it from the exit code 1. +`eslint **.js --fatal-parse-error` + +With this argument if there is at least one fatal error in the results ESLint produces it will exit with code 2. + +## Add a `fatal-parse-error` option. +We would require the option in regard to other ones as well. +``` +// inside of eslint.js + .. + .. + * @property {number} fatalErrorCount Number of fatal errors for the result. + .. + .. +``` +``` +// inside of lib/option.js +.. +.. +{ + option: "fatal-parse-error", + type: "Boolean", + default: "false", + description: "Trigger exit code 2 on any fatal errors." +} +.. +.. +``` +## Gather and report the fatal messages. + +In order for ESLint to be able to make a choice based on the fact that a `fatal` error has been found or not we must first retrieve this information. Altough we will not be needing the count of the fatal errors using count instead of a boolean and offer more flexibility than a Boolean parameter, keeps the codebase consistent. + +The changes for that are on `cli-engine.js`: +``` +function calculateStatsPerFile(messages) { + return messages.reduce((stat, message) => { + if (message.fatal || message.severity === 2) { + stat.errorCount++; + if (message.fatal) { + stat.fatalErrorCount++; + } + if (message.fix) { + stat.fixableErrorCount++; + } + } else { + stat.warningCount++; + if (message.fix) { + stat.fixableWarningCount++; + } + } + return stat; + }, { + errorCount: 0, + fatalError: 0, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0 + }); +} +``` +``` +function calculateStatsPerRun(results) { + return results.reduce((stat, result) => { + stat.errorCount += result.errorCount; + stat.fatalErrorCount += result.fatalErrorCount; + stat.warningCount += result.warningCount; + stat.fixableErrorCount += result.fixableErrorCount; + stat.fixableWarningCount += result.fixableWarningCount; + return stat; + }, { + errorCount: 0, + fatalErrorCount: 0, + warningCount: 0, + fixableErrorCount: 0, + fixableWarningCount: 0 + }); +} +``` + +## Return exit code `2` in the case of any fatal errors on `cli` + +Now we can retreve those fatal errors from `cli.js` and act accordingly: + +First change the function that retrieves those errors: +``` +function countErrors(results) { + let errorCount = 0; + let fatalErrorCount = 0; + let warningCount = 0; + + for (const result of results) { + errorCount += result.errorCount; + fatalErrorCount += result.fatalErrorCount; + warningCount += result.warningCount; + } + + return { errorCount, warningCount }; + return { errorCount, fatalErrorCount, warningCount }; +} +``` + +Now with the new information this method gives us we can use it: +``` +// on the execute method +if (await printResults(engine, results, options.format, options.outputFile)) { + const { errorCount, warningCount } = countErrors(results); + const { errorCount, fatalErrorCount, warningCount } = countErrors(results); + const tooManyWarnings = + options.maxWarnings >= 0 && warningCount > options.maxWarnings; + const fatalErrorExists = + options.fatalParseError && fatalErrorCount > 0; + + if (!errorCount && tooManyWarnings) { + log.error( + "ESLint found too many warnings (maximum: %s).", + options.maxWarnings + ); + } + + if(fatalErrorExists){ + return 2; + } + + return (errorCount || tooManyWarnings) ? 1 : 0; +} +return 2; +``` ## Documentation -It may be a good idea on why we are introducing a option that changes the way ESLint behaves. Will leave the choice to the ESLint team. +The section of the documentation requiring the change is the Command Line Interface: + +[Command Line Interface](https://eslint.org/docs/user-guide/command-line-interface) ## Drawbacks From 94ce071f98939b23e8b316e693049fa5b8367549 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Mon, 15 Feb 2021 14:17:10 +0200 Subject: [PATCH 6/9] Added more info for typescript parser and CI pipeline example --- designs/2021-break-on-parsing-errors/README.md | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 86b3e123..c44be582 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -11,7 +11,9 @@ The suggested change is for ESLint tp support an argument that will make the CLI ## Motivation -We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json or had a wrongly configured source type the error would be a `fatal` one but the exit code would be `1`. There was no distiction between a run with no fatal errors and with fatal errors. +We met with a couple of cases where we assumed a succesfull run of ESLint in CI enviroments. When ESLint wouldn't be able to read the tsconfig.json, or had a wrongly configured source type the error would be a `fatal` one but the exit code would be `1`. There was no distiction between a run with no fatal errors and with fatal errors. + +More specifically the tsconfig.json invalid read is a error from `@typescript-eslint parser`, the parser correctly indicates a fatal parsing failure like it should so it falls under the same category as the incorrectly configured `sourceType` as in both cases we can't actually run any rules in the file. According to the eslint docs about exit codes: @@ -19,9 +21,7 @@ According to the eslint docs about exit codes: - 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. -Being able to exit with code `2` instead of `1` it will allow for some CI pipelines to better understand the results and react differently if ESLint reports any `fatal` errors. - -Altough it is possible to filter out the results it will not allow the user +Being able to exit with code `2` instead of `1` it will allow for some CI pipelines to better understand the results and react differently if ESLint reports any `fatal` errors. For example, an Azure DevOps build task that is responsible for running ESLint and output a SARIF would fail on exit code `2` but not 1 indicating there is a configuration and not all of the files where scanned properly for rules. ## Detailed Design @@ -181,5 +181,14 @@ Assume that for some files ESLint successfully lints them and reports a rule but If ESLint finds a single file that has a parsing error should it report just that file or every rule as well? +## Alternatives +There 2 alternatives which we may want to consider, they were already discussed on the related issue: + +## Use a new exit code 3 +The proposal remains the same but with exit code `3` instead of `2` + +## Max Fatal Errors +Another tweak to the proposal would be for the argument to be an integer similar to `--max-warnings`. Instead of reporting a different exit code than `1` we would + ## Related Discussions https://github.com/eslint/eslint/issues/13711 From a1865c6258029b674c771c30b31cbcd55307976b Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Mon, 1 Mar 2021 02:38:45 +0200 Subject: [PATCH 7/9] Applied PR suggestion --- designs/2021-break-on-parsing-errors/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index c44be582..319bd6db 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -85,7 +85,7 @@ function calculateStatsPerFile(messages) { return stat; }, { errorCount: 0, - fatalError: 0, + fatalErrorCount: 0, warningCount: 0, fixableErrorCount: 0, fixableWarningCount: 0 From c571a351cb863e46b4b3fa4e6116cb512799a089 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Mon, 1 Mar 2021 02:49:55 +0200 Subject: [PATCH 8/9] Added the agreements for the open questions --- designs/2021-break-on-parsing-errors/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 319bd6db..8bf7c86d 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -175,11 +175,20 @@ There are no expected backward compatibility issues. The parameter will be disab ## Open Questions + +### Question: Do we want to still report all errors or only failed linting errors when `break-on-lint-error` is passed? +### Answer: +After discussions we will be reporting all errors. + +---- +### Question: Assume that for some files ESLint successfully lints them and reports a rule but for some others it doesnt. If ESLint finds a single file that has a parsing error should it report just that file or every rule as well? +### Answer: +The choice is that we will be returing all of the found linting errors as discussed in these [comments](https://github.com/eslint/rfcs/pull/76#discussion_r572924056) ## Alternatives There 2 alternatives which we may want to consider, they were already discussed on the related issue: From bec302a940abdca6df20a54ea51fd9bb29fa13c2 Mon Sep 17 00:00:00 2001 From: A-Katopodis Date: Wed, 3 Mar 2021 21:19:21 +0200 Subject: [PATCH 9/9] Changed from to fatal-parse-error to exit-on-fatal-error --- designs/2021-break-on-parsing-errors/README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/designs/2021-break-on-parsing-errors/README.md b/designs/2021-break-on-parsing-errors/README.md index 8bf7c86d..c546526a 100644 --- a/designs/2021-break-on-parsing-errors/README.md +++ b/designs/2021-break-on-parsing-errors/README.md @@ -7,7 +7,7 @@ ## Summary -The suggested change is for ESLint tp support an argument that will make the CLI exit with code 2 if any fatal errors are reported. The option will be an opt-in boolean argument called `--fatal-parse-error`. +The suggested change is for ESLint tp support an argument that will make the CLI exit with code 2 if any fatal errors are reported. The option will be an opt-in boolean argument called `--exit-on-fatal-error`. ## Motivation @@ -27,17 +27,17 @@ Being able to exit with code `2` instead of `1` it will allow for some CI pipeli Design Summary: -1. Add a `fatal-parse-error` option. +1. Add a `exit-on-fatal-error` option. 2. Gather and report the fatal messages from `cli-engine`. 3. Return exit code `2` in the case of any fatal errors on `cli` A command example: -`eslint **.js --fatal-parse-error` +`eslint **.js ----exit-on-fatal-error` With this argument if there is at least one fatal error in the results ESLint produces it will exit with code 2. -## Add a `fatal-parse-error` option. +## Add a `exit-on-fatal-error` option. We would require the option in regard to other ones as well. ``` // inside of eslint.js @@ -52,7 +52,7 @@ We would require the option in regard to other ones as well. .. .. { - option: "fatal-parse-error", + option: "exit-on-fatal-error", type: "Boolean", default: "false", description: "Trigger exit code 2 on any fatal errors."