Remove ARGV use during runtime (#65)

This PR removes `ARGV` from being used implicitly during runtime. Before
this change, ARGV would be used and modified in `Parser#parse` which
made it hard to reason about how
`ModelFilesGetter#list_model_files_from_argument` works.

`ARGV` is still used at the time of execution but gets cloned in
`Parser#new`.

Author: drwl

lib/annotate_rb/model_annotator/model_files_getter.rb

@@ -9,13 +9,9 @@ class << self
         # of model files from root dir. Otherwise we take all the model files
         # in the model_dir directory.
         def call(options)
-          model_files = []
+          model_files = list_model_files_from_argument(options)
 
-          # Note: This is currently broken as we don't set `is_rake` anywhere.
-          # It's an artifact from the old Annotate gem and how it did control flow.
-          model_files = list_model_files_from_argument(options) if !options[:is_rake]
-
-          return model_files if !model_files.empty?
+          return model_files if model_files.any?
 
           options[:model_dir].each do |dir|
             Dir.chdir(dir) do
@@ -41,9 +37,9 @@ def call(options)
         private
 
         def list_model_files_from_argument(options)
-          return [] if ARGV.empty?
+          return [] if options.get_state(:working_args).empty?
 
-          specified_files = ARGV.map { |file| File.expand_path(file) }
+          specified_files = options.get_state(:working_args).map { |file| File.expand_path(file) }
 
           model_files = options[:model_dir].flat_map do |dir|
             absolute_dir_path = File.expand_path(dir)

lib/annotate_rb/options.rb

@@ -210,5 +210,9 @@ def set_state(key, value, overwrite = false)
     def get_state(key)
       @state[key]
     end
+
+    def print
+      # TODO: prints options and state
+    end
   end
 end

lib/annotate_rb/parser.rb

@@ -35,11 +35,11 @@ def self.parse(args, existing_options = {})
     }.freeze
 
     def initialize(args, existing_options)
-      @args = args
+      @args = args.clone
       base_options = DEFAULT_OPTIONS.dup
       @options = base_options.merge(existing_options)
       @commands = []
-      @options[:original_args] = args.dup
+      @options[:original_args] = args.clone
     end
 
     def parse
@@ -52,6 +52,12 @@ def parse
       @options
     end
 
+    def remaining_args
+      # `@args` gets modified throughout the lifecycle of this class.
+      # It starts as a shallow clone of ARGV, then arguments matching commands and options are removed in #parse
+      @args
+    end
+
     private
 
     def parse_command(args)

lib/annotate_rb/runner.rb

@@ -9,14 +9,15 @@ def run(args)
     end
 
     def run(args)
-      _original_args = args.dup
-
       config_file_options = ConfigLoader.load_config
-      parsed_options = Parser.parse(args)
+      parser = Parser.new(args, {})
+
+      parsed_options = parser.parse
+      remaining_args = parser.remaining_args
 
       options = config_file_options.merge(parsed_options)
 
-      @options = Options.from(options, {})
+      @options = Options.from(options, {working_args: remaining_args})
       AnnotateRb::RakeBootstrapper.call(@options)
 
       if @options[:command]

spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb

@@ -54,7 +54,7 @@ class User < ActiveRecord::Base
       end
 
       it "displays just the error message with trace disabled (default)" do
-        options = AnnotateRb::Options.from({model_dir: @model_dir, is_rake: true})
+        options = AnnotateRb::Options.from({model_dir: @model_dir}, {working_args: []})
 
         expect { described_class.do_annotations(options) }.to output(a_string_including("Unable to process #{@model_dir}/user.rb: oops")).to_stderr
 
@@ -63,7 +63,7 @@ class User < ActiveRecord::Base
       end
 
       it "displays the error message and stacktrace with trace enabled" do
-        options = AnnotateRb::Options.from({model_dir: @model_dir, is_rake: true, trace: true})
+        options = AnnotateRb::Options.from({model_dir: @model_dir, trace: true}, {working_args: []})
         expect { described_class.do_annotations(options) }.to output(a_string_including("Unable to process #{@model_dir}/user.rb: oops")).to_stderr
 
         # TODO: Find another way of testing trace without hardcoding the file name as part of the spec
@@ -83,14 +83,14 @@ class User < ActiveRecord::Base
       end
 
       it "displays just the error message with trace disabled (default)" do
-        options = AnnotateRb::Options.from({model_dir: @model_dir, is_rake: true})
+        options = AnnotateRb::Options.from({model_dir: @model_dir}, {working_args: []})
 
         expect { described_class.remove_annotations(options) }.to output(a_string_including("Unable to process #{@model_dir}/user.rb: oops")).to_stderr
         expect { described_class.remove_annotations(options) }.not_to output(a_string_including("/user.rb:2:in `<class:User>'")).to_stderr
       end
 
       it "displays the error message and stacktrace with trace enabled" do
-        options = AnnotateRb::Options.from({model_dir: @model_dir, is_rake: true, trace: true})
+        options = AnnotateRb::Options.from({model_dir: @model_dir, trace: true}, {working_args: []})
 
         expect { described_class.remove_annotations(options) }.to output(a_string_including("Unable to process #{@model_dir}/user.rb: oops")).to_stderr
         expect { described_class.remove_annotations(options) }.to output(a_string_including("/user.rb:2:in `<class:User>'")).to_stderr

spec/lib/annotate_rb/model_annotator/model_files_getter_spec.rb

@@ -5,8 +5,6 @@
     before do
       $stdout = StringIO.new
       $stderr = StringIO.new
-
-      ARGV.clear
     end
 
     after do
@@ -30,7 +28,7 @@
       context "when the model files are not specified" do
         context "when no option is specified" do
           let(:base_options) { {model_dir: [model_dir]} }
-          let(:options) { AnnotateRb::Options.new(base_options) }
+          let(:options) { AnnotateRb::Options.new(base_options, {working_args: []}) }
 
           it "returns all model files under `model_dir` directory" do
             is_expected.to contain_exactly(
@@ -43,7 +41,7 @@
 
         context "when `ignore_model_sub_dir` option is enabled" do
           let(:base_options) { {model_dir: [model_dir], ignore_model_sub_dir: true} }
-          let(:options) { AnnotateRb::Options.new(base_options) }
+          let(:options) { AnnotateRb::Options.new(base_options, {working_args: []}) }
 
           it "returns model files just below `model_dir` directory" do
             is_expected.to contain_exactly([model_dir, "foo.rb"])
@@ -60,11 +58,9 @@
           ]
         end
 
-        before { ARGV.concat(model_files) }
-
         context "when no option is specified" do
           let(:base_options) { {model_dir: [model_dir, additional_model_dir]} }
-          let(:options) { AnnotateRb::Options.new(base_options) }
+          let(:options) { AnnotateRb::Options.new(base_options, {working_args: model_files}) }
 
           context "when all the specified files are in `model_dir` directory" do
             it "returns specified files" do
@@ -77,34 +73,21 @@
 
           context "when a model file outside `model_dir` directory is specified" do
             let(:base_options) { {model_dir: [model_dir]} }
-            let(:options) { AnnotateRb::Options.new(base_options) }
+            let(:options) { AnnotateRb::Options.new(base_options, {working_args: model_files}) }
 
             it "writes to $stderr" do
               subject
               expect($stderr.string).to include("The specified file could not be found in directory")
             end
           end
         end
-
-        context "when `is_rake` option is enabled" do
-          let(:base_options) { {model_dir: [model_dir], is_rake: true} }
-          let(:options) { AnnotateRb::Options.new(base_options) }
-
-          it "returns all model files under `model_dir` directory" do
-            is_expected.to contain_exactly(
-              [model_dir, "foo.rb"],
-              [model_dir, File.join("bar", "baz.rb")],
-              [model_dir, File.join("bar", "qux", "quux.rb")]
-            )
-          end
-        end
       end
     end
 
     context "when `model_dir` is invalid" do
       let(:model_dir) { "/not_exist_path" }
       let(:base_options) { {model_dir: [model_dir]} }
-      let(:options) { AnnotateRb::Options.new(base_options) }
+      let(:options) { AnnotateRb::Options.new(base_options, {working_args: []}) }
 
       it "writes to $stderr" do
         subject