Standardize and improve exit code handling #245

sxlijin · 2021-12-10T22:07:42Z

This PR does two things:

standardize on:
- exit(1) when markdownlint runs successfully but finds issues in the input and
- exit(2) when markdownlint fails to run successfully, perhaps because a configuration file is malformed or perhaps because some kind of filesystem error has occurred
handle errors more robustly: a runtime error inside lintAndPrint() today results in exit(1), e.g. for a file that we don't have read permissions on (note: this is not comprehensive, we do still do some code execution before lintAndPrint(), but this is still a solid improvement)

The background for this PR is that I'm working on a universal linter which makes it easy for users to run linters for different subsets of their repos, and markdownlint-cli uses exit(1) as a catch-all for not just lint findings but also unexpected failures.

Specifically we've seen markdownlint return with exit(1) when this happens:

internal/fs/utils.js:314
    throw err;
    ^

Error: ENOENT: no such file or directory, open '[sanitized]'
    at Object.openSync (fs.js:498:3)
    at Object.readFileSync (fs.js:394:35)
    at lintFile ([sanitized]/.cache/trunk/linters/markdownlint/0.29.0-ee485b2571c3867cdf56d258aa9aed05/node_modules/markdownlint/lib/markdownlint.js:714:33)
    at lintInput ([sanitized]/.cache/trunk/linters/markdownlint/0.29.0-ee485b2571c3867cdf56d258aa9aed05/node_modules/markdownlint/lib/markdownlint.js:790:7)
    at Function.markdownlintSync [as sync] ([sanitized]/.cache/trunk/linters/markdownlint/0.29.0-ee485b2571c3867cdf56d258aa9aed05/node_modules/markdownlint/lib/markdownlint.js:888:3)
    at lintAndPrint ([sanitized]/.cache/trunk/linters/markdownlint/0.29.0-ee485b2571c3867cdf56d258aa9aed05/node_modules/markdownlint-cli/markdownlint.js:324:35)
    at Object.<anonymous> ([sanitized]/.cache/trunk/linters/markdownlint/0.29.0-ee485b2571c3867cdf56d258aa9aed05/node_modules/markdownlint-cli/markdownlint.js:329:3)
    at Module._compile (internal/modules/cjs/loader.js:1072:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)
    at Module.load (internal/modules/cjs/loader.js:937:32) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '[sanitized]'
}

and this can be easily reproduced via:

touch foo.md
chmod a-r foo.md
markdownlint-cli foo.md

This PR does two things: 1. standardize on: * `exit(1)` when markdownlint runs successfully but finds issues in the input and * `exit(2)` when markdownlint fails to run successfully, perhaps because a configuration file is malformed or perhaps because some kind of filesystem error has occurred 2. handle errors more robustly: a runtime error inside `lintAndPrint()` today results in `exit(1)`, e.g. for a file that we don't have read permissions on (note: this is not comprehensive, we do still do some code execution before `lintAndPrint()`, but this is still a solid improvement)

DavidAnson

Thanks for the contribution!

It feels like this changes the meaning of exit code 2 from what is documented: https://github.com/igorshubovych/markdownlint-cli#exit-codes

Perhaps you want to create a new error code to distinguish these cases from code 1?

FYI, there seem to be issues breaking CI.

Unrelated, you may want to look at the following as it seems similar to your project: https://github.com/github/super-linter

sxlijin · 2021-12-10T23:30:47Z

It feels like this changes the meaning of exit code 2 from what is documented: https://github.com/igorshubovych/markdownlint-cli#exit-codes

Perhaps you want to create a new error code to distinguish these cases from code 1?

Ah, I somehow looked for this everywhere except in the README!

A new error code certainly makes sense.

I am slightly concerned about exit(1) being overloaded for lint findings and for a bad config though. Would you be open to splitting those error codes?

FYI, there seem to be issues breaking CI.

Ack, opened this and then went to look at something else and didn't see the CI failures. Will keep an eye on these.

Unrelated, you may want to look at the following as it seems similar to your project: https://github.com/github/super-linter

Thanks for the heads-up. I should clarify that this isn't just a personal project but we're actually a company and this is one of the products that we're building out, so this is something we're very aware of :)

DavidAnson

Left a few comments/questions on the changes.

markdownlint.js

DavidAnson · 2021-12-11T21:31:57Z

markdownlint.js

@@ -24,6 +24,13 @@ function jsYamlSafeLoad(text) {
  return require('js-yaml').load(text);
 }

+const exitCodes = {
+  lintFindings: 1,


"lintFailures", maybe?

"lintFailures" suggests to me that linting failed to happen, aka an unexpected failure, hence "lintFindings"

This one's a tricky terminology issue and is also frustratingly non standard across a lot of linters :(

lintIssues?

markdownlint.js

DavidAnson · 2021-12-11T21:35:26Z

test/test.js

@@ -126,6 +126,22 @@ test('linting of incorrect Markdown file fails with absolute path', async t => {
  }
 });

+test('linting of unreadable Markdown file fails', async t => {
+  const unreadablePath = '/tmp/unreadable.md';


Could you do relative to __dirname? I don't want to reach out like this as some systems may treat that as malware, etc..

Also, it breaks on Windows. :)

Eurghhh. This was causing race issues with the other tests, that's why I went with /tmp originally. Need to figure out somewhere it doesn't break the glob tests.

You can give it a non-standard extension? .mdtest or something?

test/test.js

DavidAnson

Couple more questions, replies to a couple of comments.

DavidAnson · 2021-12-14T07:12:54Z

markdownlint.js

-  getStdin().then(lintAndPrint);
-} else {
-  program.help();
+function main() {


What do you think about just putting this inside the try?

DavidAnson · 2021-12-14T07:14:38Z

test/test.js

+  try {
+    fs.writeFileSync(unreadablePath, '', {mode: 0o222});
+
+    try {


Not sure we need the nested try? I think the outer can have a catch and a finally?

DavidAnson · 2021-12-14T07:19:06Z

Looks like 2 CI issues:

message: 'fs.rmSync is not a function' on Node 12
Unreadable file is readable on Windows?

DavidAnson · 2021-12-14T07:21:41Z

mode seems not to be well supported on Windows, see last paragraph: https://nodejs.org/dist/latest-v16.x/docs/api/fs.html#fschmodpath-mode-callback

sxlijin · 2021-12-14T08:37:52Z

maybe a deliberately broken symlink will work on windows?

DavidAnson

Thanks!

sxlijin · 2021-12-16T19:40:55Z

and thanks to you for being so quick and responsive!

DavidAnson requested changes Dec 10, 2021

View reviewed changes

sxlijin added 2 commits December 10, 2021 15:32

address feedback

912f8b9

fix tests

91db24c

sxlijin requested a review from DavidAnson December 10, 2021 23:52

document the change in exit code

7ddae77

DavidAnson requested changes Dec 11, 2021

View reviewed changes

address next round of feedback

78b3042

DavidAnson requested changes Dec 14, 2021

View reviewed changes

sxlijin added 2 commits December 14, 2021 00:28

feedback

874e5a8

use a symlink instead of a-r for the test file

6fa4e5a

DavidAnson approved these changes Dec 16, 2021

View reviewed changes

DavidAnson merged commit c5a8b48 into igorshubovych:master Dec 16, 2021

sxlijin deleted the sam/standardized-exit-codes branch December 16, 2021 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize and improve exit code handling #245

Standardize and improve exit code handling #245

sxlijin commented Dec 10, 2021

DavidAnson left a comment

sxlijin commented Dec 10, 2021

DavidAnson left a comment

DavidAnson Dec 11, 2021

sxlijin Dec 13, 2021

DavidAnson Dec 14, 2021

DavidAnson Dec 11, 2021

DavidAnson Dec 11, 2021

sxlijin Dec 13, 2021

DavidAnson Dec 14, 2021

DavidAnson left a comment

DavidAnson Dec 14, 2021

sxlijin Dec 14, 2021

DavidAnson Dec 14, 2021

DavidAnson commented Dec 14, 2021

DavidAnson commented Dec 14, 2021

sxlijin commented Dec 14, 2021

DavidAnson left a comment

sxlijin commented Dec 16, 2021

Standardize and improve exit code handling #245

Standardize and improve exit code handling #245

Conversation

sxlijin commented Dec 10, 2021

DavidAnson left a comment

Choose a reason for hiding this comment

sxlijin commented Dec 10, 2021

DavidAnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidAnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidAnson commented Dec 14, 2021

DavidAnson commented Dec 14, 2021

sxlijin commented Dec 14, 2021

DavidAnson left a comment

Choose a reason for hiding this comment

sxlijin commented Dec 16, 2021