diff --git a/lib/annotate_rb/model_annotator/model_wrapper.rb b/lib/annotate_rb/model_annotator/model_wrapper.rb index 12c7864b..e546723d 100644 --- a/lib/annotate_rb/model_annotator/model_wrapper.rb +++ b/lib/annotate_rb/model_annotator/model_wrapper.rb @@ -6,6 +6,8 @@ class ModelWrapper # Should be the wrapper for an ActiveRecord model that serves as the source of truth of the model # of the model that we're annotating + DEFAULT_TIMESTAMP_COLUMNS = %w[created_at updated_at] + def initialize(klass, options) @klass = klass @options = options @@ -143,10 +145,13 @@ def classified_sort(cols) associations = [] id = nil + # specs don't load defaults, so ensure we have defaults here + timestamp_columns = @options[:timestamp_columns] || DEFAULT_TIMESTAMP_COLUMNS + cols.each do |c| if c.name.eql?("id") id = c - elsif c.name.eql?("created_at") || c.name.eql?("updated_at") + elsif timestamp_columns.include?(c.name) timestamps << c elsif c.name[-3, 3].eql?("_id") associations << c @@ -154,7 +159,10 @@ def classified_sort(cols) rest_cols << c end end - [rest_cols, timestamps, associations].each { |a| a.sort_by!(&:name) } + + timestamp_order = timestamp_columns.each_with_index.to_h + timestamps.sort_by! { |col| timestamp_order[col.name] } + [rest_cols, associations].each { |a| a.sort_by!(&:name) } ([id] << rest_cols << timestamps << associations).flatten.compact end diff --git a/lib/annotate_rb/options.rb b/lib/annotate_rb/options.rb index 45b7724a..2d7df57f 100644 --- a/lib/annotate_rb/options.rb +++ b/lib/annotate_rb/options.rb @@ -10,7 +10,7 @@ class Options class << self def from(options = {}, state = {}) - new(options, state).load_defaults + new(options, state) end end @@ -36,7 +36,6 @@ def from(options = {}, state = {}) exclude_sti_subclasses: false, # ModelAnnotator exclude_tests: false, # ModelAnnotator force: false, # ModelAnnotator, but should be used by both - format_bare: true, # Unused format_markdown: false, # ModelAnnotator, RouteAnnotator format_rdoc: false, # ModelAnnotator format_yard: false, # ModelAnnotator @@ -68,6 +67,9 @@ def from(options = {}, state = {}) # ModelAnnotator hide_limit_column_types: "", + # ModelAnnotator + timestamp_columns: ModelAnnotator::ModelWrapper::DEFAULT_TIMESTAMP_COLUMNS, + ignore_columns: nil, # ModelAnnotator ignore_routes: nil, # RouteAnnotator ignore_unknown_models: false, # ModelAnnotator @@ -92,71 +94,10 @@ def from(options = {}, state = {}) DEFAULT_OPTIONS = {}.merge(POSITION_OPTIONS, FLAG_OPTIONS, OTHER_OPTIONS, PATH_OPTIONS).freeze - FLAG_OPTION_KEYS = [ - :classified_sort, - :exclude_controllers, - :exclude_factories, - :exclude_fixtures, - :exclude_helpers, - :exclude_scaffolds, - :exclude_serializers, - :exclude_sti_subclasses, - :exclude_tests, - :force, - :format_markdown, - :format_rdoc, - :format_yard, - :frozen, - :ignore_model_sub_dir, - :ignore_unknown_models, - :include_version, - :show_check_constraints, - :show_complete_foreign_keys, - :show_foreign_keys, - :show_indexes, - :simple_indexes, - :sort, - :timestamp, - :trace, - :with_comment, - :with_column_comments, - :with_table_comments - ].freeze - - OTHER_OPTION_KEYS = [ - :active_admin, - :command, - :debug, - :hide_default_column_types, - :hide_limit_column_types, - :ignore_columns, - :ignore_routes, - :ignore_unknown_models, - :models, - :routes, - :skip_on_db_migrate, - :target_action, - :wrapper, - :wrapper_close, - :wrapper_open, - :classes_default_to_s - ].freeze - - PATH_OPTION_KEYS = [ - :additional_file_patterns, - :model_dir, - :require, - :root_dir - ].freeze - - ALL_OPTION_KEYS = [ - POSITION_OPTIONS.keys, FLAG_OPTION_KEYS, OTHER_OPTION_KEYS, PATH_OPTION_KEYS - ].flatten.freeze - POSITION_DEFAULT = "before" # Want this to be read only after initializing - def_delegator :@options, :[] + def_delegators :@options, :[], :to_h def initialize(options = {}, state = {}) @options = options @@ -164,30 +105,50 @@ def initialize(options = {}, state = {}) # For now, state is a hash to store state that we need but is not a configuration option @state = state - symbolize_exclude_tests + load_defaults + @options.freeze + end + + def set_state(key, value, overwrite = false) + if @state.key?(key) && !overwrite + val = @state[key] + raise ArgumentError, "Attempting to write '#{value}' to state with key '#{key}', but it already exists with '#{val}'." + end + + @state[key] = value end - def to_h - @options.to_h + def get_state(key) + @state[key] end + def print + # TODO: prints options and state + end + + private + def load_defaults - ALL_OPTION_KEYS.each do |key| - @options[key] = DEFAULT_OPTIONS[key] unless @options.key?(key) + @options = DEFAULT_OPTIONS.merge(@options) + + # `:exclude_tests` option is being expanded to function as a boolean OR an array of symbols + # https://github.com/drwl/annotaterb/issues/103 + if @options[:exclude_tests].is_a?(Array) + @options[:exclude_tests].map! { |item| item.to_s.strip.to_sym } end # Set all of the position options in the following order: # 1) Use the value if it's defined # 2) Use value from :position if it's defined # 3) Use default - POSITION_OPTIONS.keys.each do |key| + POSITION_OPTIONS.each_key do |key| @options[key] = Helper.fallback( @options[key], @options[:position], POSITION_DEFAULT ) end # Unpack path options if we're passed in a String - PATH_OPTION_KEYS.each do |key| + PATH_OPTIONS.each_key do |key| if @options[key].is_a?(String) @options[key] = @options[key].split(",").map(&:strip).reject(&:empty?) end @@ -200,41 +161,6 @@ def load_defaults # Set column and table comments to default to :with_comment, if not set @options[:with_column_comments] = @options[:with_comment] if @options[:with_column_comments].nil? @options[:with_table_comments] = @options[:with_comment] if @options[:with_table_comments].nil? - - self - end - - def set_state(key, value, overwrite = false) - if @state.key?(key) && !overwrite - val = @state[key] - raise ArgumentError, "Attempting to write '#{value}' to state with key '#{key}', but it already exists with '#{val}'." - end - - @state[key] = value - end - - def get_state(key) - @state[key] - end - - def print - # TODO: prints options and state - end - - private - - # Guard against user inputting strings instead of symbols - def symbolize_exclude_tests - # `:exclude_tests` option is being expanded to function as a boolean OR an array of symbols - # https://github.com/drwl/annotaterb/issues/103 - - if @options[:exclude_tests].is_a?(Array) - @options[:exclude_tests].map! do |item| - item = item.strip if item.respond_to?(:strip) - - item.to_sym - end - end end end end diff --git a/spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb b/spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb index 1bbf6d8a..8a29e2f5 100644 --- a/spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb +++ b/spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb @@ -22,7 +22,7 @@ class User < ActiveRecord::Base end it "works with namespaced models (i.e. models inside modules/subdirectories)" do - (model_file_name, file_content) = write_model "foo/user.rb", <<~EOS + (model_file_name, file_content) = write_model(File.join("foo", "user.rb"), <<~EOS) class Foo::User < ActiveRecord::Base end EOS @@ -42,58 +42,61 @@ class Foo::User < ActiveRecord::Base expect(File.read(model_file_name)).to eq("#{schema_info}#{file_content}") end - describe "if a file can't be annotated" do - before do - allow(AnnotateRb::ModelAnnotator::ModelClassGetter).to receive(:get_loaded_model_by_path).with("user").and_return(nil) + context "when an error occurs" do + let(:basename) { "user" } + let(:error_message) { a_string_including("Unable to process #{File.join(@model_dir, "#{basename}.rb")}: oops") } + let(:stacktrace) { a_string_including(%r{(^.*:\d+:in [`'].*'$\n?)+}m) } - write_model("user.rb", <<~EOS) - class User < ActiveRecord::Base - raise "oops" - end - EOS - end + describe "while annotating" do + before do + allow(AnnotateRb::ModelAnnotator::ModelClassGetter).to receive(:get_loaded_model_by_path).with(basename).and_return(nil) - it "displays just the error message with trace disabled (default)" do - options = AnnotateRb::Options.from({model_dir: @model_dir}, {working_args: []}) + write_model("#{basename}.rb", <<~EOS) + class #{basename.classify} < ActiveRecord::Base + raise "oops" + end + EOS + end - expect { described_class.do_annotations(options) }.to output(a_string_including("Unable to process #{@model_dir}/user.rb: oops")).to_stderr + it "displays just the error message with trace disabled (default)" do + options = AnnotateRb::Options.from({model_dir: @model_dir}, {working_args: []}) - # TODO: Find another way of testing trace without hardcoding the file name as part of the spec - # expect { described_class.do_annotations(options) }.not_to output(a_string_including('/spec/annotate/annotate_models_spec.rb:')).to_stderr - end + expect { described_class.do_annotations(options) }.to output(error_message).to_stderr + expect { described_class.do_annotations(options) }.not_to output(stacktrace).to_stderr + end - it "displays the error message and stacktrace with trace enabled" do - 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 + it "displays the error message and stacktrace with trace enabled" do + options = AnnotateRb::Options.from({model_dir: @model_dir, trace: true}, {working_args: []}) - # TODO: Find another way of testing trace without hardcoding the file name as part of the spec - # expect { described_class.do_annotations(options) }.to output(a_string_including('/spec/lib/annotate/annotate_models_spec.rb:')).to_stderr + expect { described_class.do_annotations(options) }.to output(error_message).to_stderr + expect { described_class.do_annotations(options) }.to output(stacktrace).to_stderr + end end - end - describe "if a file can't be deannotated" do - before do - allow(AnnotateRb::ModelAnnotator::ModelClassGetter).to receive(:get_loaded_model_by_path).with("user").and_return(nil) + describe "while removing annotations" do + before do + allow(AnnotateRb::ModelAnnotator::ModelClassGetter).to receive(:get_loaded_model_by_path).with(basename).and_return(nil) - write_model("user.rb", <<~EOS) - class User < ActiveRecord::Base - raise "oops" - end - EOS - end + write_model("#{basename}.rb", <<~EOS) + class #{basename.classify} < ActiveRecord::Base + raise "oops" + end + EOS + end - it "displays just the error message with trace disabled (default)" do - options = AnnotateRb::Options.from({model_dir: @model_dir}, {working_args: []}) + it "displays just the error message with trace disabled (default)" do + 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 `'")).to_stderr - end + expect { described_class.remove_annotations(options) }.to output(error_message).to_stderr + expect { described_class.remove_annotations(options) }.not_to output(stacktrace).to_stderr + end - it "displays the error message and stacktrace with trace enabled" do - options = AnnotateRb::Options.from({model_dir: @model_dir, trace: true}, {working_args: []}) + it "displays the error message and stacktrace with trace enabled" do + 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 `'")).to_stderr + expect { described_class.remove_annotations(options) }.to output(error_message).to_stderr + expect { described_class.remove_annotations(options) }.to output(stacktrace).to_stderr + end end end end diff --git a/spec/lib/annotate_rb/model_annotator/annotation/annotation_builder_spec.rb b/spec/lib/annotate_rb/model_annotator/annotation/annotation_builder_spec.rb index 49c6a4ae..35bddb05 100644 --- a/spec/lib/annotate_rb/model_annotator/annotation/annotation_builder_spec.rb +++ b/spec/lib/annotate_rb/model_annotator/annotation/annotation_builder_spec.rb @@ -181,6 +181,60 @@ it 'works with option "classified_sort"' do is_expected.to eq expected_result end + + context "when default timestamps are included" do + let(:columns) do + [ + mock_column("parent_id", :integer), + mock_column("updated_at", :datetime), + mock_column("name", :string), + mock_column("id", :integer), + mock_column("deleted_at", :datetime), + mock_column("created_at", :datetime) + ] + end + + it "sorts default timestamps second last before associations" do + is_expected.to eq <<~EOS + # == Schema Information + # + # Table name: users + # + # id :integer not null, primary key + # deleted_at :datetime not null + # name :string not null + # created_at :datetime not null + # updated_at :datetime not null + # parent_id :integer not null + # + EOS + end + + context "when timestamps_column option is set" do + let(:options) do + AnnotateRb::Options.new( + classified_sort: true, + timestamp_columns: %w[created_at updated_at deleted_at] + ) + end + + it "sorts configured timestamps into config order" do + is_expected.to eq <<~EOS + # == Schema Information + # + # Table name: users + # + # id :integer not null, primary key + # name :string not null + # created_at :datetime not null + # updated_at :datetime not null + # deleted_at :datetime not null + # parent_id :integer not null + # + EOS + end + end + end end context "when geometry columns are included" do diff --git a/spec/lib/annotate_rb/options_spec.rb b/spec/lib/annotate_rb/options_spec.rb index ad5eceaa..d40e339f 100644 --- a/spec/lib/annotate_rb/options_spec.rb +++ b/spec/lib/annotate_rb/options_spec.rb @@ -1,18 +1,17 @@ # frozen_string_literal: true RSpec.describe AnnotateRb::Options do + let(:options) { {} } + let(:state) { {} } + describe ".from" do subject { described_class.from(options, state) } - let(:options) { {} } - let(:state) { {} } - it { is_expected.to be_a(described_class) } end describe ".new" do subject { described_class.new(options, state) } - let(:state) { {} } context ":exclude_tests option is an Array of strings" do let(:options) { {exclude_tests: ["serializers", "controllers"]} } @@ -51,16 +50,9 @@ it { expect(subject[:exclude_tests]).to eq(false) } end - end - - describe ".load_defaults" do - subject { described_class.new(options, state).load_defaults } - - let(:state) { {} } context 'when default value of "show_complete_foreign_keys" is not set' do let(:key) { :show_complete_foreign_keys } - let(:options) { {} } it "returns false" do expect(subject[key]).to eq(false) @@ -76,10 +68,14 @@ end end + describe "default timestamp_columns" do + it "sets defaults matching Rails' convention" do + expect(subject[:timestamp_columns]).to match_array(%w[created_at updated_at]) + end + end + describe "comment options" do context "when using defaults" do - let(:options) { {} } - it "uses the defaults" do expect(subject[:with_comment]).to eq(true) expect(subject[:with_column_comments]).to eq(true)