feat: better Error handling (#284)

* feat(cli): add --quiet / -q option

Stops any successful output. Errors are still outputted.

* refactor(cli): create output functions

This commit creates stdout and stderr functions for outputting with
respect to isQuiet and adds support for differing (easier to parse)
messages if not connected to a TTY (eg. a pipe or process substitution)

Additionally changes the 'No matching files.' message to output on stderr
rather than on stdout as it was, which makes sense because this output
represents erroneous usage.

* fix(cli): change stderr to not respect --quiet

* fix(cli): change stdout to stderr in isTerminal

* fix(tests): add STD(OUT/ERR)_IS_TTY env variables

these variables can be set to force output on the specified chanel
as if it were connected to a terminal.
macro.testCLI can now take an argument of the following form:
isTerminal: { stderr: false, stdout: false },

* fix(cli): improve output of CLI when not a tty

* test: added tests for --verson and --help

* test: add tests for --quiet

* test: include isTerminal in snapshot

* test: add tests for TTY detection and integration

* test: typo, stderr --> stdout

* fix(cli): exit code while sort is number of failed

The exit code is now the number of files which failed when sorting.
On a successful run, this will be 0 and exit w/ success, but if any
errors occur, this will be incremented. In check mode, this is fails
+ unsorted.

* fix: wrap file operation in try catch

Wraps fs calls in trycatch to not throw and to count failures. Also better messages and script usage

* docs: document changes to tty-based output

* fix(test): compatability w/ node v14

* fix: compatability with node 12

* test: add tests for improper usage

* test: support for node 12&14 in error test

* refactor: remove extra changes to improve diff

* revert: remove all tty detection

* refactor: update to meet upstream

* fix: fixes error reporting with quiet

* fix: bad merge

* style: prettier

* fix: fixes permissions on cli.js

* typo

* refactor: improve exit code handling, and set 2 on error

* feat: added summary at end of tool run

* fix: better show that output on error, is an error

* refactor: cleaner logic

* refactor: pass `files` to `constructor`

* refactor: save `matchedFilesCount` to status

* fix: remove `0 files`, use `1 file` instead of `1 files`

* refactor: extract `Reporter`

---------

Co-authored-by: Keith Cirkel <[email protected]>
Co-authored-by: fisker Cheung <[email protected]>

cli.js

@@ -2,6 +2,7 @@
 import { globbySync } from 'globby'
 import fs from 'node:fs'
 import sortPackageJson from './index.js'
+import Reporter from './reporter.js'
 
 function showVersion() {
   const { name, version } = JSON.parse(
@@ -26,52 +27,34 @@ If file/glob is omitted, './package.json' file will be processed.
   )
 }
 
-function sortPackageJsonFiles(patterns, { isCheck, shouldBeQuiet }) {
-  const files = globbySync(patterns)
-  const printToStdout = shouldBeQuiet ? () => {} : console.log
-
-  if (files.length === 0) {
-    console.error('No matching files.')
-    process.exitCode = 2
-    return
+function sortPackageJsonFile(file, reporter, isCheck) {
+  const original = fs.readFileSync(file, 'utf8')
+  const sorted = sortPackageJson(original)
+  if (sorted === original) {
+    return reporter.reportNotChanged(file)
   }
 
-  let notSortedFiles = 0
-  for (const file of files) {
-    const packageJson = fs.readFileSync(file, 'utf8')
-    const sorted = sortPackageJson(packageJson)
-
-    if (sorted !== packageJson) {
-      if (isCheck) {
-        notSortedFiles++
-        printToStdout(file)
-        process.exitCode = 1
-      } else {
-        fs.writeFileSync(file, sorted)
-
-        printToStdout(`${file} is sorted!`)
-      }
-    }
+  if (!isCheck) {
+    fs.writeFileSync(file, sorted)
   }
 
-  if (isCheck) {
-    // Print a empty line
-    printToStdout()
+  reporter.reportChanged(file)
+}
+
+function sortPackageJsonFiles(patterns, options) {
+  const files = globbySync(patterns)
+  const reporter = new Reporter(files, options)
+  const { isCheck } = options
 
-    if (notSortedFiles) {
-      printToStdout(
-        notSortedFiles === 1
-          ? `${notSortedFiles} of ${files.length} matched file is not sorted.`
-          : `${notSortedFiles} of ${files.length} matched files are not sorted.`,
-      )
-    } else {
-      printToStdout(
-        files.length === 1
-          ? `${files.length} matched file is sorted.`
-          : `${files.length} matched files are sorted.`,
-      )
+  for (const file of files) {
+    try {
+      sortPackageJsonFile(file, reporter, isCheck)
+    } catch (error) {
+      reporter.reportFailed(file, error)
     }
   }
+
+  reporter.printSummary()
 }
 
 function run() {

package.json

@@ -24,7 +24,8 @@
   "files": [
     "index.js",
     "index.d.ts",
-    "cli.js"
+    "cli.js",
+    "reporter.js"
   ],
   "scripts": {
     "lint": "eslint .",

reporter.js

@@ -0,0 +1,120 @@
+const getFilesCountText = (count) => (count === 1 ? '1 file' : `${count} files`)
+
+class Reporter {
+  #hasPrinted = false
+  #options
+  #status
+  #logger
+
+  constructor(files, options) {
+    this.#options = options
+    this.#status = {
+      matchedFilesCount: files.length,
+      failedFilesCount: 0,
+      wellSortedFilesCount: 0,
+      changedFilesCount: 0,
+    }
+
+    this.#logger = options.shouldBeQuiet
+      ? { log() {}, error() {} }
+      : {
+          log: (...args) => {
+            this.#hasPrinted = true
+            console.log(...args)
+          },
+          error: (...args) => {
+            this.#hasPrinted = true
+            console.error(...args)
+          },
+        }
+  }
+
+  // The file is well-sorted
+  reportNotChanged(/* file */) {
+    this.#status.wellSortedFilesCount++
+  }
+
+  reportChanged(file) {
+    this.#status.changedFilesCount++
+    this.#logger.log(this.#options.isCheck ? `${file}` : `${file} is sorted!`)
+  }
+
+  reportFailed(file, error) {
+    this.#status.failedFilesCount++
+
+    console.error('Error on: ' + file)
+    this.#logger.error(error.message)
+  }
+
+  printSummary() {
+    const {
+      matchedFilesCount,
+      failedFilesCount,
+      changedFilesCount,
+      wellSortedFilesCount,
+    } = this.#status
+
+    if (matchedFilesCount === 0) {
+      console.error('No matching files.')
+      process.exitCode = 2
+      return
+    }
+
+    const { isCheck, isQuiet } = this.#options
+
+    if (isCheck && changedFilesCount) {
+      process.exitCode = 1
+    }
+
+    if (failedFilesCount) {
+      process.exitCode = 2
+    }
+
+    if (isQuiet) {
+      return
+    }
+
+    const { log } = this.#logger
+
+    // Print an empty line.
+    if (this.#hasPrinted) {
+      log()
+    }
+
+    // Matched files
+    log('Found %s.', getFilesCountText(matchedFilesCount))
+
+    // Failed files
+    if (failedFilesCount) {
+      log(
+        '%s could not be %s.',
+        getFilesCountText(failedFilesCount),
+        isCheck ? 'checked' : 'sorted',
+      )
+    }
+
+    // Changed files
+    if (changedFilesCount) {
+      if (isCheck) {
+        log(
+          '%s %s not sorted.',
+          getFilesCountText(changedFilesCount),
+          changedFilesCount === 1 ? 'was' : 'were',
+        )
+      } else {
+        log('%s successfully sorted.', getFilesCountText(changedFilesCount))
+      }
+    }
+
+    // Well-sorted files
+    if (wellSortedFilesCount) {
+      log(
+        '%s %s already sorted.',
+        getFilesCountText(wellSortedFilesCount),
+        wellSortedFilesCount === 1 ? 'was' : 'were',
+      )
+    }
+  }
+}
+
+export default Reporter

tests/cli.js

@@ -1,5 +1,5 @@
-import fs from 'node:fs'
 import test from 'ava'
+import fs from 'node:fs'
 import { cliScript, macro } from './_helpers.js'
 
 const badJson = {
@@ -438,3 +438,53 @@ test('run `cli --check --quiet` on duplicate patterns', macro.testCLI, {
   ],
   message: 'Should not count `bad-1/package.json` more than once. Exit code 1',
 })
+
+const badFormat = ''
+
+test('run `cli --check` on 1 non-json file', macro.testCLI, {
+  fixtures: [
+    {
+      file: 'notJson/package.json',
+      content: badFormat,
+      expect: badFormat,
+    },
+  ],
+  args: ['*/package.json', '--check'],
+  message: 'Should fail to check, but not end execution.',
+})
+
+test('run `cli --check --quiet` on 1 non-json file', macro.testCLI, {
+  fixtures: [
+    {
+      file: 'notJson/package.json',
+      content: badFormat,
+      expect: badFormat,
+    },
+  ],
+  args: ['*/package.json', '--check', '--quiet'],
+  message: 'Should output error message, but not count.',
+})
+
+test('run `cli` on 1 non-json file', macro.testCLI, {
+  fixtures: [
+    {
+      file: 'notJson/package.json',
+      content: badFormat,
+      expect: badFormat,
+    },
+  ],
+  args: ['*/package.json'],
+  message: 'Should fail to check, but not end execution.',
+})
+
+test('run `cli --quiet` on 1 non-json file', macro.testCLI, {
+  fixtures: [
+    {
+      file: 'notJson/package.json',
+      content: badFormat,
+      expect: badFormat,
+    },
+  ],
+  args: ['*/package.json', '--quiet'],
+  message: 'Should output error message',
+})