Custom exit code for format change

Daniel Ma · asterite · commit 068bf0b417f4 · 2017-07-12T13:48:51.000+02:00
While working on [rufo.el](https://github.com/danielma/rufo.el), I ran into a problem with rufo's exit codes, and I'd like to get it sorted out before submitting a PR. Right now, [rufo sends the exit code `1` for a successful format](https://github.com/asterite/rufo/blob/master/lib/rufo/command.rb#L30), but it also [exits with `1` for a syntax error](https://github.com/asterite/rufo/blob/master/lib/rufo/command.rb#L36). As a plugin author, this makes it a little difficult to encode proper behavior, since a `1` is indeterminate. Looking at [this table of exit codes](https://tldp.org/LDP/abs/html/exitcodes.html), it seems like `1` should be reserved for errors (like a syntax error), but 3-125 are free game for custom exit codes. This PR makes it so that rufo exits with | Code | Result | | ---- | ---- | | `0` | No errors, but also no formatting changes | | `1` | Error. Either `Ripper` could not parse syntax or input file is missing | | `3` | Input changed. Formatted code differs from input |
diff --git a/README.md b/README.md
@@ -141,7 +141,15 @@ $ rufo --check file names or dir names
 ```
 
 This will print one line for each file that isn't correctly formatted
-according to **Rufo**, and will exit with exit code 1.
+according to **Rufo**, and will exit with exit code 3.
+
+### Exit codes
+
+| Code | Result |
+| ---- | ---- |
+| `0` | No errors, but also no formatting changes |
+| `1` | Error. Either `Ripper` could not parse syntax or input file is missing |
+| `3` | Input changed. Formatted code differs from input |
 
 ## Editor support
 
diff --git a/lib/rufo/command.rb b/lib/rufo/command.rb
@@ -3,6 +3,10 @@
 require "optparse"
 
 class Rufo::Command
+  CODE_OK = 0
+  CODE_ERROR = 1
+  CODE_CHANGE = 3
+
   def self.run(argv)
     want_check, filename_for_dot_rufo = parse_options(argv)
     new(want_check, filename_for_dot_rufo).run(argv)
@@ -15,25 +19,28 @@ def initialize(want_check, filename_for_dot_rufo)
   end
 
   def run(argv)
-    if argv.empty?
-      format_stdin
-    else
-      format_args argv
-    end
+    status_code = if argv.empty?
+                    format_stdin
+                  else
+                    format_args argv
+                  end
+
+    exit status_code.to_i
   end
 
   def format_stdin
     code = STDIN.read
+
+    return CODE_ERROR if code.strip.empty?
+
     result = format(code, @filename_for_dot_rufo || Dir.getwd)
 
-    if @want_check
-      exit 1 if result != code
-    else
-      print result
-    end
+    print(result) if !@want_check
+
+    CODE_CHANGE if code != result
   rescue Rufo::SyntaxError
     STDERR.puts "Error: the given text is not a valid ruby program (it has syntax errors)"
-    exit 1
+    CODE_ERROR
   rescue => ex
     STDERR.puts "You've found a bug!"
     STDERR.puts "Please report it to https://github.com/asterite/rufo/issues with code that triggers it"
@@ -54,13 +61,22 @@ def format_args(args)
       end
     end
 
+    return CODE_ERROR if files.empty?
+
     changed = false
+    syntax_error = false
 
     files.each do |file|
-      changed |= format_file file
+      result = format_file(file)
+
+      changed |= result == CODE_CHANGE
+      syntax_error |= result == CODE_ERROR
     end
 
-    exit 1 if changed
+    case
+    when syntax_error then CODE_ERROR
+    when changed      then CODE_CHANGE
+    end
   end
 
   def format_file(filename)
@@ -72,24 +88,22 @@ def format_file(filename)
       # We ignore syntax errors as these might be template files
       # with .rb extension
       STDERR.puts "Error: #{filename} has syntax errors"
-      return true
+      return CODE_ERROR
     end
 
     if code.force_encoding(result.encoding) != result
       if @want_check
-        STDERR.puts "Error: formatting #{filename} produced changes"
+        STDERR.puts "Formatting #{filename} produced changes"
       else
         File.write(filename, result)
         puts "Format: #{filename}"
       end
 
-      return true
+      return CODE_CHANGE
     end
-
-    false
   rescue Rufo::SyntaxError
     STDERR.puts "Error: the given text in #{filename} is not a valid ruby program (it has syntax errors)"
-    exit 1
+    CODE_ERROR
   rescue => ex
     STDERR.puts "You've found a bug!"
     STDERR.puts "It happened while trying to format the file #{filename}"
diff --git a/spec/command_spec.rb b/spec/command_spec.rb
@@ -0,0 +1,160 @@
+require "spec_helper"
+require "tempfile"
+
+RSpec.describe Rufo::Command do
+  describe "exit codes" do
+    matcher :terminate do
+      actual = nil
+
+      match do |block|
+        begin
+          block.call
+        rescue SystemExit => e
+          actual = e.status
+        end
+        actual and actual == status_code
+      end
+
+      chain :with_code do |status_code|
+        @status_code = status_code
+      end
+
+      failure_message do |block|
+        "expected block to call exit(#{status_code}) but exit" +
+          (actual.nil? ? " not called" : "(#{actual}) was called")
+      end
+
+      failure_message_when_negated do |block|
+        "expected block not to call exit(#{status_code})"
+      end
+
+      description do
+        "exit(#{status_code})"
+      end
+
+      def status_code
+        @status_code ||= 0
+      end
+    end
+
+    describe "files" do
+      describe "check" do
+        subject { ->{ described_class.run(["-c", file]) } }
+
+        context "missing file" do
+          let(:file) { "missing.rb" }
+          it { is_expected.to terminate.with_code(1) }
+        end
+
+        context "unchanged file" do
+          let(:file) { "spec/fixtures/valid" }
+          it { is_expected.to terminate }
+        end
+
+        context "changed file" do
+          let(:file) { "spec/fixtures/needs_changes" }
+          it { is_expected.to terminate.with_code 3 }
+        end
+
+        context "invalid file" do
+          let(:file) { "spec/fixtures/syntax_error" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+      end
+
+      describe "format" do
+        subject do
+          lambda do
+            tempfile = File.exist?(file) && Tempfile.new("rufo-fixture")
+
+            if tempfile
+              begin
+                tempfile.write(File.read(file))
+                tempfile.rewind
+                described_class.run([tempfile.path])
+              rescue SystemExit
+                tempfile.close
+                tempfile.unlink
+                raise
+              end
+            else
+              described_class.run([file])
+            end
+          end
+        end
+
+        context "missing file" do
+          let(:file) { "missing.rb" }
+          it { is_expected.to terminate.with_code(1) }
+        end
+
+        context "unchanged file" do
+          let(:file) { "spec/fixtures/valid" }
+          it { is_expected.to terminate }
+        end
+
+        context "changed file" do
+          let(:file) { "spec/fixtures/needs_changes" }
+          it { is_expected.to terminate.with_code 3 }
+        end
+
+        context "invalid file" do
+          let(:file) { "spec/fixtures/syntax_error" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+      end
+    end
+
+    describe "STDIN" do
+      describe "check" do
+        subject { ->{ described_class.run(["-c"]) } }
+        before(:example) { expect(STDIN).to receive(:read).and_return code }
+
+        context "no code" do
+          let(:code) { "" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+
+        context "unchanged code" do
+          let(:code) { "[]\n" }
+          it { is_expected.to terminate }
+        end
+
+        context "changed code" do
+          let(:code) { "[ ]\n" }
+          it { is_expected.to terminate.with_code 3 }
+        end
+
+        context "invalid code" do
+          let(:code) { "not_valid(ruby" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+      end
+
+      describe "format" do
+        subject { ->{ described_class.run([]) } }
+        before(:example) { expect(STDIN).to receive(:read).and_return code }
+
+        context "no code" do
+          let(:code) { "" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+
+        context "unchanged code" do
+          let(:code) { "[]\n" }
+          it { is_expected.to terminate }
+        end
+
+        context "changed code" do
+          let(:code) { "[ ]\n" }
+          it { is_expected.to terminate.with_code 3 }
+        end
+
+        context "invalid code" do
+          let(:code) { "not_valid(ruby" }
+          it { is_expected.to terminate.with_code 1 }
+        end
+      end
+    end
+  end
+end
diff --git a/spec/fixtures/needs_changes b/spec/fixtures/needs_changes
@@ -0,0 +1,4 @@
+class  NeedsChanges
+  def a_method( with_bizarre_formatting)
+  end
+end
diff --git a/spec/fixtures/syntax_error b/spec/fixtures/syntax_error
@@ -0,0 +1 @@
+invalid(ruby_syntax
diff --git a/spec/fixtures/valid b/spec/fixtures/valid
@@ -0,0 +1,3 @@
+def valid_ruby(argument)
+  argument + 2
+end

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +class  NeedsChanges
 +  def a_method( with_bizarre_formatting)
 +  end
 +end
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+def valid_ruby(argument)`
	`2`	`+ argument + 2`
	`3`	`+end`