Fix annotation placement in nested files and around trailing user comments#334
Merged
Merged
Conversation
annotaterb's parser-level analysis of a model file uses two pieces of
output from `CustomParser` directly: the contiguous block of comments
adjacent to the schema header (to find an existing annotation), and the
list of `block_starts` (to choose where a new annotation should anchor).
Both are too naive on their own and lead to wrong placements in two
real-world scenarios.
## Existing annotation absorbs trailing user comments
When a model is documented with a comment block placed after the schema
annotation, `AnnotationFinder` previously walked backward through the
contiguous comment block looking for the first `#` separator. If the
user's documentation contains a `#` line between paragraphs, the walk
terminates inside the user comment block instead of at the schema's
trailing `#`. The first paragraph plus the inner separator are then
absorbed into the detected annotation range and lost on regeneration.
The same logic mis-detects the end when an annotation lacks the
canonical trailing `#` (e.g. someone trimmed it manually): the walk
back stops at a section-internal `#` (between `# Foreign Keys` and the
FK row), and the actual last content row escapes the range and gets
duplicated on the next run.
Replace the walk-back with `walk_forward_through_schema`, which
advances from the `== Schema Info` start while every successor still
matches `schema_like?` (header prefix, header-exact match, tabular row
with two-or-more leading spaces, or `#` separator). It stops at the
last schema-like comment, regardless of whether the file ends with a
trailing `#`, a content row, or has user prose appended below.
## Insertion target is wrong in nested files
`Generator#determine_annotation_position` and
`ParsedFile#annotation_position` previously used `parsed.starts.first`
or `parsed.starts.select { type_map[name] == :class }.last`. Both are
too naive:
- `parsed.starts.first` treats `require 'rails_helper'` (or any leading
`require_relative` / `load`) as a block start, so an annotation
inserted with `nested_position` lands above the require line in spec
files.
- `select { :class }.last` picks the last *occurrence* of any class
name. If a model references its own class inside a method body
(e.g. `Sample::Helper.run`), the last occurrence is the method-body
reference and the annotation gets inserted inside the method.
Introduce `FileParser::AnnotationTarget.find` as the single entry point
for "what is the annotation target line?". It rejects `require` /
`require_relative` / `load` and de-duplicates class entries by name,
so each class contributes exactly its declaration line. Both
`Generator` and `ParsedFile` go through it, which keeps insertion
location and `annotation_position` consistent.
yamat47
added a commit
to yamat47/annotaterb
that referenced
this pull request
May 6, 2026
drwl#334 introduced `AnnotationTarget` and made it pick the deepest class declaration when `nested_position` is on. That heuristic is wrong for models containing inner class declarations (custom errors, value objects, Struct/Data classes, …): the deepest declaration is the inner one, and the schema annotation gets inserted directly above it instead of above the model class. ```ruby class Sample < ApplicationRecord include SomeMixin class ProcessingError < StandardError; end # ← deepest, wrongly chosen def call raise ProcessingError end end ``` `ProjectAnnotator` already resolves each model file to its actual ActiveRecord class via `ModelClassGetter`, so the authoritative class name is available without any heuristic. Thread `klass.name.demodulize` through `SingleFileAnnotatorInstruction` → `SingleFileAnnotator` → `ParsedFile`/`Generator` → `AnnotationTarget.find` as `model_class_name:`, and have the helper match that name against `parser.type_map` first. The deepest-declaration heuristic remains as a fallback for when the name is absent (related files like specs and factories) or doesn't match anything in the source (legacy file/class layouts).
drwl
approved these changes
May 8, 2026
Owner
drwl
left a comment
There was a problem hiding this comment.
Thanks for the write up and making these changes!
yamat47
added a commit
to yamat47/annotaterb
that referenced
this pull request
May 9, 2026
drwl#334 introduced `AnnotationTarget` and made it pick the deepest class declaration when `nested_position` is on. That heuristic is wrong for models containing inner class declarations (custom errors, value objects, Struct/Data classes, …): the deepest declaration is the inner one, and the schema annotation gets inserted directly above it instead of above the model class. ```ruby class Sample < ApplicationRecord include SomeMixin class ProcessingError < StandardError; end # ← deepest, wrongly chosen def call raise ProcessingError end end ``` `ProjectAnnotator` already resolves each model file to its actual ActiveRecord class via `ModelClassGetter`, so the authoritative class name is available without any heuristic. Thread `klass.name.demodulize` through `SingleFileAnnotatorInstruction` → `SingleFileAnnotator` → `ParsedFile`/`Generator` → `AnnotationTarget.find` as `model_class_name:`, and have the helper match that name against `parser.type_map` first. The deepest-declaration heuristic remains as a fallback for when the name is absent (related files like specs and factories) or doesn't match anything in the source (legacy file/class layouts).
OdenTakashi
pushed a commit
that referenced
this pull request
May 11, 2026
…_position` (#336) ## Summary When `nested_position` is enabled, `AnnotationTarget` (introduced in #334) picks the **deepest** class declaration as the insertion point. That heuristic breaks for any model that contains an inner class declaration — custom errors, value objects, `Struct`/`Data` classes, etc. — because the deepest declaration is the inner one, so the schema annotation gets inserted directly above it instead of above the model class itself. ### Reproduction ```ruby class Sample < ApplicationRecord include SomeMixin class ProcessingError < StandardError; end # ← deepest, wrongly chosen def call raise ProcessingError end end ``` Running annotaterb with nested_position lands the schema block right above class ProcessingError, not above class Sample. ## Fix `ProjectAnnotator` already resolves each model file to its actual `ActiveRecord` class via `ModelClassGetter`, so we have the authoritative class name without needing any heuristic. This PR threads `klass.name.demodulize` through: `SingleFileAnnotatorInstruction` → `SingleFileAnnotator` → `ParsedFile` / `AnnotatedFile::Generator` → `FileParser::AnnotationTarget.find` as `model_class_name`:, and `AnnotationTarget.find` matches that name against `parser.type_map` first. The deepest-declaration heuristic remains as a fallback for the cases where the name is absent or doesn't match anything in the source: - related files (specs, factories, fixtures) don't get a `model_class_name` - legacy file/class layouts where the declared class name doesn't match the resolved AR class name So existing behavior is preserved everywhere except the inner-class case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two real-world layouts make annotaterb misplace or corrupt the schema annotation.
1. The end of an existing annotation gets mis-detected
AnnotationFinderwalks backward looking for the first#separator. With a multi-paragraph user doc placed after the schema, the walk stops inside the user's doc:The first paragraph and the inner
#are absorbed into the detected annotation range and silently lost when the annotation is rewritten.The same logic fails when the annotation lacks its trailing
#separator (e.g. manually trimmed): the walk stops at a section-internal#, and the trailing content row escapes the detected range — duplicating on the next run.2. The annotation target line is wrong in nested files
Generator#determine_annotation_positionpreviously usedparsed.starts.first(no class case) andparsed.starts.select { |name, _| type_map[name] == :class }.last(class case). Both fail on common shapes:Solution
Two small post-processors over
CustomParser's raw output.walk_forward_through_schema(inAnnotationFinder, replaces the walk-back). Advances from the schema header while every successor matchesschema_like?— a header prefix, an exact header (Indexes / Foreign Keys / Check Constraints), a tabular row (#+ 2+ leading spaces), or a#separator. Stops at the last schema-like comment.FileParser::AnnotationTarget.find(new). Skipsrequire/require_relative/loadand de-duplicates class entries by name so each class only contributes its declaration line.GeneratorandParsedFileboth go through it.