Merge pull request #134 from solhint-community/cli-breaking-changes

exit with a different code when the linter is configured incorrectly

CHANGELOG.md

@@ -1,3 +1,11 @@
+## [4.0.0-rc02] - XXXX-XX-XX
+
+### Breaking
+- exit with a different code when linter is configured incorrectly (255) vs
+  when errors are found in linted files (1) https://github.com/solhint-community/solhint-community/pull/134
+    - also exit eagerly when a misconfiguration is detected, to help the programmer
+    realize of their mistake sooner
+
 ## [4.0.0-rc01] - 2024-01-28
 
 ### Breaking

README.md

@@ -86,6 +86,12 @@ Commands:
   list-rules                              display enabled rules of current config, including extensions
 ```
 
+## Exit codes
+
+- `0`: linted files had no errors
+- `1`: linted files had 1 or more errors, or more warnings than `--max-warnings`
+- `255`: provided command-line options were invalid, see stderr for details
+
 ## Configuration
 
 You can use a `.solhint.json` file to configure Solhint for the whole project.

e2e/formatters.js

@@ -4,6 +4,16 @@ const { useFixture } = require('./utils')
 
 describe('formatters', function () {
   useFixture('03-no-empty-blocks')
+  it('non existent formatter', async function () {
+    const { stdout, stderr, code } = shell.exec(
+      'solhint Foo.sol --formatter hexadecimal-spaceship',
+      { silent: true }
+    )
+    expect(code).to.eq(255) // code for bad options
+    expect(stdout.trim()).to.eq('')
+    expect(stderr).to.include('There was a problem loading formatter option')
+  })
+
   it('unix', async function () {
     const { stdout } = shell.exec('solhint Foo.sol --formatter unix', { silent: true })
     const lines = stdout.split('\n')

e2e/ignores.js

@@ -9,15 +9,15 @@ describe('excludedFiles in config file is used', function () {
   describe('GIVEN a .solhintignore ignoring a file AND a custom config ignoring another', function () {
     describe('WHEN linting with a glob that matches both', function () {
       beforeEach(function () {
-        ;({ code, stdout } = shell.exec(`solhint -c ignoreInConfig.json '*.sol'`, {
+        ;({ code, stdout } = shell.exec(`solhint -c ignoreInConfig.json 'Ignored*.sol'`, {
           silent: true,
         }))
       })
       it('THEN no errors are reported on stdout', function () {
         expect(stdout.trim()).to.eq('')
       })
-      it('AND the program exits with error code 0', function () {
-        expect(code).to.eq(0)
+      it('AND the program exits with error code 255 since no files were linted', function () {
+        expect(code).to.eq(255)
       })
     })
   })
@@ -46,17 +46,17 @@ describe('.solhintignore in subdirectory is not read', function () {
 describe('excludeFiles in config is relative to cwd and not config file location', function () {
   useFixture('12-solhintignore')
   let code
-  let stdout
+  let stderr
   describe('GIVEN a subdirectory with its own config file ignoring said subdirectory', function () {
     describe('WHEN linting there with a root-level cwd', function () {
       beforeEach(function () {
-        ;({ code, stdout } = shell.exec(`solhint -c sub/subconfig.json 'sub/*.sol'`, {
+        ;({ code, stderr } = shell.exec(`solhint -c sub/subconfig.json 'sub/*.sol'`, {
           silent: true,
         }))
       })
       it('THEN the file is ignored', function () {
-        expect(code).to.eq(0)
-        expect(stdout.trim()).to.eq('')
+        expect(code).to.eq(255)
+        expect(stderr).to.include('No files to lint')
       })
     })
   })
@@ -79,17 +79,17 @@ describe('--ignore-path is used', function () {
           ))
         })
         it('THEN an error is printed to stderr', function () {
-          expect(stderr).to.include('ERROR: does-not-exist is not a valid path.')
+          expect(stderr).to.include(
+            'ERROR: custom ignore file not found at provided path does-not-exist.'
+          )
         })
-        // FIXME: we should exit early on v4
-        it('AND no ignore file is used, so errors are reported on the file', function () {
-          expect(stdout).to.include('1 problem')
-          expect(stdout).to.include('IgnoredDefault')
+        it('AND the linter exits early, not reporting anything', function () {
+          expect(stdout.trim()).to.eq('')
         })
         // FIXME: we should use a different error code for bad
         // settings/parameters on v4
-        it('AND the program exits with error code 1', function () {
-          expect(code).to.eq(1)
+        it('AND the program exits with error code 255 for bad options', function () {
+          expect(code).to.eq(255)
         })
       })
 
@@ -103,14 +103,15 @@ describe('--ignore-path is used', function () {
           ))
         })
         it('THEN an error is printed to stderr', function () {
-          expect(stderr).to.include('ERROR: does-not-exist is not a valid path.')
+          expect(stderr).to.include(
+            'ERROR: custom ignore file not found at provided path does-not-exist.'
+          )
         })
         it('AND no errors are reported on stdout', function () {
           expect(stdout.trim()).to.eq('')
         })
-        // FIXME: I seriously do not agree with this. Change on v4
-        it('AND the program exits with error code 0', function () {
-          expect(code).to.eq(0)
+        it('AND the program exits with error code 255 for bad options', function () {
+          expect(code).to.eq(255)
         })
       })
     })
@@ -120,13 +121,13 @@ describe('--ignore-path is used', function () {
       // it needlessly hard for editor integrations
       describe('AND passing an ignored file as a parameter', function () {
         beforeEach(function () {
-          ;({ code, stdout } = shell.exec('solhint IgnoredDefault.sol', {
+          ;({ code, stderr } = shell.exec('solhint IgnoredDefault.sol', {
             silent: true,
           }))
         })
         it('THEN the file is ignored', function () {
-          expect(code).to.eq(0)
-          expect(stdout.trim()).to.equal('')
+          expect(code).to.eq(255)
+          expect(stderr).to.include('No files to lint')
         })
       })
 
@@ -168,16 +169,16 @@ describe('--ignore-path is used', function () {
 
       describe('AND passing an ignored file as a parameter', function () {
         beforeEach(function () {
-          ;({ code, stdout } = shell.exec(
+          ;({ code, stderr } = shell.exec(
             'solhint --ignore-path other-solhintignore IgnoredOther.sol',
             {
               silent: true,
             }
           ))
         })
         it('THEN the file is ignored', function () {
-          expect(code).to.eq(0)
-          expect(stdout.trim()).to.equal('')
+          expect(code).to.eq(255)
+          expect(stderr).to.include('No files to lint')
         })
       })
 

e2e/test.js

@@ -24,7 +24,7 @@ describe('main executable tests', function () {
     it('should fail', function () {
       const { code } = shell.exec('solhint Foo.sol', { silent: true })
 
-      expect(code).to.equal(1)
+      expect(code).to.equal(255)
     })
 
     it('should create an initial config with init-config', function () {
@@ -53,15 +53,33 @@ describe('main executable tests', function () {
     let code
     let stderr
     let stdout
+
+    describe('GIVEN a non-existing extra config file passed with -c WHEN linting', function () {
+      beforeEach(function () {
+        ;({ code, stderr, stdout } = shell.exec('solhint -c nothere.json Foo.sol', {
+          silent: true,
+        }))
+      })
+
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
+      })
+      it('AND stdout is empty', function () {
+        expect(stdout.trim()).to.eq('')
+      })
+      it('AND stderr logs the file wasnt found', function () {
+        expect(stderr.trim()).to.include('Extra config file "nothere.json" couldnt be found')
+      })
+    })
     describe('GIVEN a config file with invalid syntax, WHEN linting', function () {
       beforeEach(function () {
         ;({ code, stderr, stdout } = shell.exec('solhint -c broken-json-syntax.json Foo.sol', {
           silent: true,
         }))
       })
 
-      it('THEN linter exits with error 1', function () {
-        expect(code).to.equal(1)
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
       })
       it('AND stdout is empty', function () {
         expect(stdout.trim()).to.eq('')
@@ -79,8 +97,8 @@ describe('main executable tests', function () {
           }))
         })
 
-        it('THEN linter exits with error 1', function () {
-          expect(code).to.equal(1)
+        it('THEN linter exits with error 255 for bad options', function () {
+          expect(code).to.equal(255)
         })
         it('AND stdout is empty', function () {
           expect(stdout.trim()).to.eq('')
@@ -101,8 +119,8 @@ describe('main executable tests', function () {
         }))
       })
 
-      it('THEN linter exits with error 1', function () {
-        expect(code).to.equal(1)
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
       })
       it('AND stdout is empty', function () {
         expect(stdout.trim()).to.eq('')
@@ -120,8 +138,8 @@ describe('main executable tests', function () {
         }))
       })
 
-      it('THEN linter exits with error 1', function () {
-        expect(code).to.equal(1)
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
       })
       it('AND stdout is empty', function () {
         expect(stdout.trim()).to.eq('')
@@ -139,8 +157,8 @@ describe('main executable tests', function () {
         }))
       })
 
-      it('THEN linter exits with error 1', function () {
-        expect(code).to.equal(1)
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
       })
       it('AND stdout is empty', function () {
         expect(stdout.trim()).to.eq('')
@@ -158,8 +176,8 @@ describe('main executable tests', function () {
         }))
       })
 
-      it('THEN linter exits with error 1', function () {
-        expect(code).to.equal(1)
+      it('THEN linter exits with error 255 for bad options', function () {
+        expect(code).to.equal(255)
       })
       it('AND stdout is empty', function () {
         expect(stdout.trim()).to.eq('')
@@ -177,18 +195,16 @@ describe('main executable tests', function () {
     it('should print an error', function () {
       const { code, stdout, stderr } = shell.exec('solhint Foo.sol', { silent: true })
 
-      expect(code).to.equal(1)
+      expect(code).to.equal(255)
 
       expect(stdout.trim()).to.equal('')
       expect(stderr).to.include('ConfigMissingError')
     })
 
-    it('should show warning when using init-config', function () {
+    it('should exit with bad options code and print an error when calling init-config', function () {
       const { code, stdout } = shell.exec('solhint init-config', { silent: true })
-
-      expect(code).to.equal(0)
-
-      expect(stdout.trim()).to.equal('Configuration file already exists')
+      expect(stdout.trim()).to.contain('Configuration file already exists')
+      expect(code).to.eq(255)
     })
   })
 
@@ -312,6 +328,33 @@ describe('main executable tests', function () {
     })
   })
 
+  describe('no files to lint', function () {
+    let code
+    let stderr
+    let stdout
+    useFixture('03-no-empty-blocks')
+    describe('WHEN calling solhint with no arguments', function () {
+      beforeEach(function () {
+        ;({ code, stdout } = shell.exec('solhint', { silent: true }))
+      })
+      it('THEN prints a help message AND reports no error', function () {
+        expect(code).to.eq(0)
+        expect(stdout).to.include('Linter for Solidity programming language')
+      })
+    })
+
+    describe('WHEN calling solhint with an argument matching zero files', function () {
+      beforeEach(function () {
+        ;({ code, stderr } = shell.exec('solhint wrongfile.sol', { silent: true }))
+      })
+      it('THEN it exits with bad options code AND reports an error on stderr', function () {
+        expect(code).to.eq(255)
+        expect(stderr).to.include('No files to lint')
+        expect(stderr).to.include('check glob arguments "wrongfile.sol"')
+      })
+    })
+  })
+
   describe('list-rules ', function () {
     let code
     let stdout
@@ -369,11 +412,13 @@ describe('main executable tests', function () {
             { silent: true }
           ))
         })
-        it('THEN it returns error code 1', function () {
-          expect(code).to.equal(1)
+        it('THEN it returns error code 255 for bad options', function () {
+          expect(code).to.equal(255)
         })
-        it('AND it reports the problem to stderr', function () {
-          expect(stderr).to.contain("Failed to load a solhint's config file")
+        it('AND stderr logs the file wasnt found', function () {
+          expect(stderr.trim()).to.include(
+            'Extra config file "config-file-with-weird-name.json" couldnt be found'
+          )
         })
         it('AND it does NOT list disabled rules', function () {
           expect(stdout).to.eq('')