Update column pattern regex to incorporate special column comments (#23)

* Extracts `AnnotationDiffGenerator` out of `FileAnnotator`
* Updates the column pattern regex based on
ctran/annotate_models#946
* Adds some tests for `FileAnnotator`

Author: drwl

lib/annotate_rb/model_annotator.rb

@@ -26,5 +26,7 @@ module ModelAnnotator
     autoload :RelatedFilesListBuilder, 'annotate_rb/model_annotator/related_files_list_builder'
     autoload :AnnotationDecider, 'annotate_rb/model_annotator/annotation_decider'
     autoload :FileAnnotatorInstruction, 'annotate_rb/model_annotator/file_annotator_instruction'
+    autoload :AnnotationDiffGenerator, 'annotate_rb/model_annotator/annotation_diff_generator'
+    autoload :AnnotationDiff, 'annotate_rb/model_annotator/annotation_diff'
   end
 end

lib/annotate_rb/model_annotator/annotation_diff.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+module AnnotateRb
+  module ModelAnnotator
+    # Plain old Ruby object for holding the differences
+    class AnnotationDiff
+      attr_reader :current_columns, :new_columns
+
+      def initialize(current_columns, new_columns)
+        @current_columns = current_columns.dup.freeze
+        @new_columns = new_columns.dup.freeze
+      end
+
+      def changed?
+        @changed ||= @current_columns != @new_columns
+      end
+    end
+  end
+end

lib/annotate_rb/model_annotator/annotation_diff_generator.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module AnnotateRb
+  module ModelAnnotator
+    # Compares the current file content and new annotation block and generates the column annotation differences
+    class AnnotationDiffGenerator
+      HEADER_PATTERN = /(^# Table name:.*?\n(#.*[\r]?\n)*[\r]?)/.freeze
+      COLUMN_PATTERN = /^#[\t ]+[\w\*\.`\[\]():]+[\t ]+.+$/.freeze
+
+      class << self
+        def call(file_content, annotation_block)
+          new(file_content, annotation_block).generate
+        end
+      end
+
+      # @param [String] file_content The current file content of the model file we intend to annotate
+      # @param [String] annotation_block The annotation block we intend to write to the model file
+      def initialize(file_content, annotation_block)
+        @file_content = file_content
+        @annotation_block = annotation_block
+      end
+
+      def generate
+        # Ignore the Schema version line because it changes with each migration
+        current_annotations = @file_content.match(HEADER_PATTERN).to_s
+        new_annotations = @annotation_block.match(HEADER_PATTERN).to_s
+
+        if current_annotations.present?
+          current_annotation_columns = current_annotations.scan(COLUMN_PATTERN).sort
+        else
+          current_annotation_columns = []
+        end
+
+        if new_annotations.present?
+          new_annotation_columns = new_annotations.scan(COLUMN_PATTERN).sort
+        else
+          new_annotation_columns = []
+        end
+
+        _result = AnnotationDiff.new(current_annotation_columns, new_annotation_columns)
+      end
+    end
+  end
+end

lib/annotate_rb/model_annotator/file_annotator.rb

@@ -24,18 +24,12 @@ def call_with_instructions(instruction)
         def call(file_name, info_block, position, options = {})
           return false unless File.exist?(file_name)
           old_content = File.read(file_name)
-          return false if old_content =~ /#{Constants::SKIP_ANNOTATION_PREFIX}.*\n/
 
-          # Ignore the Schema version line because it changes with each migration
-          header_pattern = /(^# Table name:.*?\n(#.*[\r]?\n)*[\r]?)/
-          old_header = old_content.match(header_pattern).to_s
-          new_header = info_block.match(header_pattern).to_s
+          return false if old_content =~ /#{Constants::SKIP_ANNOTATION_PREFIX}.*\n/
 
-          column_pattern = /^#[\t ]+[\w\*\.`]+[\t ]+.+$/
-          old_columns = old_header && old_header.scan(column_pattern).sort
-          new_columns = new_header && new_header.scan(column_pattern).sort
+          diff = AnnotationDiffGenerator.new(old_content, info_block).generate
 
-          return false if old_columns == new_columns && !options[:force]
+          return false if !diff.changed? && !options[:force]
 
           abort "AnnotateRb error. #{file_name} needs to be updated, but annotaterb was run with `--frozen`." if options[:frozen]
 

spec/lib/annotate_rb/annotate_models/annotate_models_annotating_a_file_spec.rb

@@ -26,8 +26,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation before class if :position == 'before'" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@schema_info}#{@file_content}")
+        expect(File.read(@model_file_name)).to eq("#{@schema_info}#{@file_content}")
       end
     end
 
@@ -36,8 +35,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation before class if :position == :before" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@schema_info}#{@file_content}")
+        expect(File.read(@model_file_name)).to eq("#{@schema_info}#{@file_content}")
       end
     end
 
@@ -46,8 +44,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation before class if :position == 'top'" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@schema_info}#{@file_content}")
+        expect(File.read(@model_file_name)).to eq("#{@schema_info}#{@file_content}")
       end
     end
 
@@ -56,8 +53,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation before class if :position == :top" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@schema_info}#{@file_content}")
+        expect(File.read(@model_file_name)).to eq("#{@schema_info}#{@file_content}")
       end
     end
 
@@ -66,8 +62,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation after class if position: 'after'" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@file_content}\n#{@schema_info}")
+        expect(File.read(@model_file_name)).to eq("#{@file_content}\n#{@schema_info}")
       end
     end
 
@@ -76,8 +71,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation after class if position: :after" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@file_content}\n#{@schema_info}")
+        expect(File.read(@model_file_name)).to eq("#{@file_content}\n#{@schema_info}")
       end
     end
 
@@ -86,8 +80,7 @@ class User < ActiveRecord::Base
 
       it "should put annotation after class if position: 'bottom'" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@file_content}\n#{@schema_info}")
+        expect(File.read(@model_file_name)).to eq("#{@file_content}\n#{@schema_info}")
       end
     end
 
@@ -96,15 +89,13 @@ class User < ActiveRecord::Base
 
       it "should put annotation after class if position: :bottom" do
         annotate_one_file position: position
-        expect(File.read(@model_file_name))
-          .to eq("#{@file_content}\n#{@schema_info}")
+        expect(File.read(@model_file_name)).to eq("#{@file_content}\n#{@schema_info}")
       end
     end
 
     it 'should wrap annotation if wrapper is specified' do
       annotate_one_file wrapper_open: 'START', wrapper_close: 'END'
-      expect(File.read(@model_file_name))
-        .to eq("# START\n#{@schema_info}# END\n#{@file_content}")
+      expect(File.read(@model_file_name)).to eq("# START\n#{@schema_info}# END\n#{@file_content}")
     end
 
     describe 'with existing annotation' do

spec/lib/annotate_rb/model_annotator/annotation_diff_generator_spec.rb

@@ -0,0 +1,146 @@
+# frozen_string_literal: true
+
+RSpec.describe AnnotateRb::ModelAnnotator::AnnotationDiffGenerator do
+  def test_columns_match_expected
+    remove_whitespace = Proc.new { |str| str.delete(" \t\r\n")  }
+
+    resulting_current_columns_data = subject.current_columns.map(&remove_whitespace)
+    expected_current_columns_data = current_columns.map(&remove_whitespace)
+    resulting_new_columns_data = subject.new_columns.map(&remove_whitespace)
+    expected_new_columns_data = new_columns.map(&remove_whitespace)
+
+    expect(resulting_current_columns_data).to eq(expected_current_columns_data)
+    expect(resulting_new_columns_data).to eq(expected_new_columns_data)
+  end
+
+  describe '.call' do
+    subject { described_class.call(file_content, annotation_block) }
+
+    context 'when model file does not have any annotations' do
+      let(:file_content) do
+        <<~FILE
+          class User < ApplicationRecord
+          end
+        FILE
+      end
+      let(:annotation_block) do
+        <<~ANNOTATION
+          # == Schema Information
+          #
+          # Table name: users
+          #
+          #  id                     :bigint           not null, primary key
+          #
+        ANNOTATION
+      end
+
+      let(:current_columns) do
+        []
+      end
+      let(:new_columns) do
+        [
+          "#  id                     :bigint           not null, primary key",
+          "# Table name: users"
+        ]
+      end
+
+      it 'returns an AnnotationDiff object with the expected old and new columns' do
+        test_columns_match_expected
+
+        expect(subject.changed?).to eq(true)
+      end
+    end
+
+    context 'when model files has the latest annotations (does not need to be updated)' do
+      let(:file_content) do
+        <<~FILE
+          # == Schema Information
+          #
+          # Table name: users
+          #
+          #  id                     :bigint           not null, primary key
+          #
+          class User < ApplicationRecord
+          end
+        FILE
+      end
+      let(:annotation_block) do
+        <<~ANNOTATION
+          # == Schema Information
+          #
+          # Table name: users
+          #
+          #  id                     :bigint           not null, primary key
+          #
+        ANNOTATION
+      end
+
+      let(:current_columns) do
+        [
+          "#  id                     :bigint           not null, primary key",
+          "# Table name: users"
+        ]
+      end
+      let(:new_columns) do
+        [
+          "#  id                     :bigint           not null, primary key",
+          "# Table name: users"
+        ]
+      end
+
+      it 'returns an AnnotationDiff object with the expected old and new columns' do
+        test_columns_match_expected
+
+        expect(subject.changed?).to eq(false)
+      end
+    end
+
+    context 'when model file has existing annotations with column comments' do
+      let(:annotation_block) do
+        <<~SCHEMA
+          # == Schema Information
+          #
+          # Table name: users
+          #
+          #  id                          :bigint           not null, primary key
+          #  name([sensitivity: medium]) :string(50)       not null
+          #
+        SCHEMA
+      end
+      let(:file_content) do
+        <<~FILE
+          # == Schema Information
+          #
+          # Table name: users
+          #
+          #  id                       :bigint           not null, primary key
+          #  name([sensitivity: low]) :string(50)       not null
+          #
+          class User < ApplicationRecord
+          end
+        FILE
+      end
+
+      let(:current_columns) do
+        [
+          "#  id                       :bigint           not null, primary key",
+          "#  name([sensitivity: low]) :string(50)       not null",
+          "# Table name: users"
+        ]
+      end
+      let(:new_columns) do
+        [
+          "#  id                          :bigint           not null, primary key",
+          "#  name([sensitivity: medium]) :string(50)       not null",
+          "# Table name: users"
+        ]
+      end
+
+      it 'returns an AnnotationDiff object with the expected old and new columns' do
+        test_columns_match_expected
+
+        expect(subject.changed?).to eq(true)
+      end
+    end
+  end
+end

spec/lib/annotate_rb/model_annotator/annotation_diff_spec.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+RSpec.describe AnnotateRb::ModelAnnotator::AnnotationDiff do
+  describe 'attributes' do
+    subject { described_class.new(current_columns, new_columns) }
+    let(:current_columns) { 'some current columns string' }
+    let(:new_columns) { 'some new columns string' }
+
+    it 'returns the current columns' do
+      expect(subject.current_columns).to eq(current_columns)
+    end
+
+    it 'returns the new columns' do
+      expect(subject.new_columns).to eq(new_columns)
+    end
+  end
+
+  describe '#changed?' do
+    subject { described_class.new(current_columns, new_columns).changed? }
+
+    context 'when the current and new columns are the same' do
+      let(:current_columns) { 'the same' }
+      let(:new_columns) { 'the same' }
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'when the current and new columns are different' do
+      let(:current_columns) { 'the current' }
+      let(:new_columns) { 'the new' }
+
+      it { is_expected.to eq(true) }
+    end
+  end
+end