From d3e4706d499cb7625102e953cca7430eaf7babf4 Mon Sep 17 00:00:00 2001 From: Eduardo Alencar Date: Wed, 27 May 2026 14:13:37 -0300 Subject: [PATCH] Remove annotations from related files regardless of exclude_* options When a related file type is excluded (e.g. `exclude_tests: true`), `annotaterb models --delete` skipped those files, leaving previously written annotations orphaned with no built-in way to clean them up. ProjectAnnotationRemover reuses RelatedFilesListBuilder, which gates each related type on the `exclude_*` options for both annotation and removal. Add a `for_removal:` flag (default false) that bypasses those guards so removal reaches every related file; the annotate path is unchanged. Removing an annotation from a file that has none is a no-op. Refs #340 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../project_annotation_remover.rb | 2 +- .../related_files_list_builder.rb | 30 +++++++++------- .../related_files_list_builder_spec.rb | 35 +++++++++++++++++++ 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/lib/annotate_rb/model_annotator/project_annotation_remover.rb b/lib/annotate_rb/model_annotator/project_annotation_remover.rb index 16348cd8..63cda9de 100644 --- a/lib/annotate_rb/model_annotator/project_annotation_remover.rb +++ b/lib/annotate_rb/model_annotator/project_annotation_remover.rb @@ -48,7 +48,7 @@ def build_instructions_for_file(file) model_instruction = SingleFileRemoveAnnotationInstruction.new(file, @options) instructions << model_instruction - related_files = RelatedFilesListBuilder.new(file, model_name, table_name, @options).build + related_files = RelatedFilesListBuilder.new(file, model_name, table_name, @options, for_removal: true).build related_file_instructions = related_files.map do |f, _position_key| _instruction = SingleFileRemoveAnnotationInstruction.new(f, @options) end diff --git a/lib/annotate_rb/model_annotator/related_files_list_builder.rb b/lib/annotate_rb/model_annotator/related_files_list_builder.rb index 0b6024a9..bb5607f0 100644 --- a/lib/annotate_rb/model_annotator/related_files_list_builder.rb +++ b/lib/annotate_rb/model_annotator/related_files_list_builder.rb @@ -10,27 +10,33 @@ class RelatedFilesListBuilder # Valid options when `:exclude_tests` is an Array, note that symbols are expected EXCLUDE_TEST_OPTIONS = %i[model controller serializer request routing].freeze - def initialize(file, model_name, table_name, options) + def initialize(file, model_name, table_name, options, for_removal: false) @file = file @model_name = model_name @table_name = table_name @options = options + # When removing annotations we want to reach every related file the gem + # could have annotated, regardless of the current `exclude_*` settings. + # Those options control where annotations are *added*; excluding a file + # type should not strand an annotation that was written before the + # exclusion was configured. See ProjectAnnotationRemover. + @for_removal = for_removal end def build @list = [] - add_related_test_files if !exclude_model_test_files? - add_related_fixture_files if !@options[:exclude_fixtures] - add_related_factory_files if !@options[:exclude_factories] - add_related_serializer_files if !@options[:exclude_serializers] - add_related_serializer_test_files if !exclude_serializer_tests? - add_related_controller_test_files if !exclude_controller_tests? - add_related_request_spec_files if !exclude_request_specs? - add_related_routing_spec_files if !exclude_routing_specs? - add_related_controller_files if !@options[:exclude_controllers] - add_related_helper_files if !@options[:exclude_helpers] - add_related_admin_files if @options[:active_admin] + add_related_test_files if @for_removal || !exclude_model_test_files? + add_related_fixture_files if @for_removal || !@options[:exclude_fixtures] + add_related_factory_files if @for_removal || !@options[:exclude_factories] + add_related_serializer_files if @for_removal || !@options[:exclude_serializers] + add_related_serializer_test_files if @for_removal || !exclude_serializer_tests? + add_related_controller_test_files if @for_removal || !exclude_controller_tests? + add_related_request_spec_files if @for_removal || !exclude_request_specs? + add_related_routing_spec_files if @for_removal || !exclude_routing_specs? + add_related_controller_files if @for_removal || !@options[:exclude_controllers] + add_related_helper_files if @for_removal || !@options[:exclude_helpers] + add_related_admin_files if @for_removal || @options[:active_admin] add_additional_file_patterns if @options[:additional_file_patterns].present? @list.uniq diff --git a/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb b/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb index f86b2cff..a1411023 100644 --- a/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb +++ b/spec/lib/annotate_rb/model_annotator/related_files_list_builder_spec.rb @@ -76,6 +76,41 @@ it { is_expected.to be_empty } end + context "when building for removal with every related type excluded", :isolated_environment do + # `exclude_*` controls where annotations are *added*. When removing, we must + # still reach those files, otherwise annotations written before a file type + # was excluded are stranded and `--delete` can never clean them up. + subject { described_class.new(file, model_name, table_name, options, for_removal: true).build } + + let(:options) { AnnotateRb::Options.new(**include_nothing_options) } + let(:model_name) { "test_default" } + + before do + FileUtils.mkdir_p("spec/models") + FileUtils.touch("spec/models/test_default_spec.rb") + + FileUtils.mkdir_p("spec/fixtures") + FileUtils.touch("spec/fixtures/test_defaults.yml") + + FileUtils.mkdir_p("spec/factories") + FileUtils.touch("spec/factories/test_default_factory.rb") + + FileUtils.mkdir_p("app/helpers") + FileUtils.touch("app/helpers/test_defaults_helper.rb") + end + + it "still lists the related files so their annotations can be removed" do + files = subject.map(&:first) + + expect(files).to include( + "spec/models/test_default_spec.rb", + "spec/fixtures/test_defaults.yml", + "spec/factories/test_default_factory.rb", + "app/helpers/test_defaults_helper.rb" + ) + end + end + context "when including model tests", :isolated_environment do let(:options) { AnnotateRb::Options.new(**include_nothing_options.merge({exclude_tests: exclude_tests_option})) } let(:exclude_tests_option) { false }