Skip to content

Fix annotation placement in nested files and around trailing user comments#334

Merged
drwl merged 1 commit into
drwl:mainfrom
yamat47:fix/annotation-boundary-and-target
May 8, 2026
Merged

Fix annotation placement in nested files and around trailing user comments#334
drwl merged 1 commit into
drwl:mainfrom
yamat47:fix/annotation-boundary-and-target

Conversation

@yamat47
Copy link
Copy Markdown
Contributor

@yamat47 yamat47 commented May 6, 2026

Problem

Two real-world layouts make annotaterb misplace or corrupt the schema annotation.

1. The end of an existing annotation gets mis-detected

AnnotationFinder walks backward looking for the first # separator. With a multi-paragraph user doc placed after the schema, the walk stops inside the user's doc:

# == Schema Information
# Table name: users
# ...
#                                  # ← actual end of schema
# First paragraph of user doc.
#                                  # ← walk-back stops HERE (wrong)
# Second paragraph of user doc.
class User < ApplicationRecord

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_position previously used parsed.starts.first (no class case) and parsed.starts.select { |name, _| type_map[name] == :class }.last (class case). Both fail on common shapes:

# spec file: parsed.starts.first picks `require`,
# so the annotation lands above the require line:
# == Schema Information       # ← wrong: should be just above RSpec.describe
# ...
require 'rails_helper'

RSpec.describe Sample do
end
# self-reference inside the method body: `.last` picks
# the body reference, so the annotation lands inside the method:
class Sample < ApplicationRecord
  def call
    # == Schema Information    # ← wrong: should be above `class Sample`
    # ...
    Sample::Helper.run
  end
end

Solution

Two small post-processors over CustomParser's raw output.

walk_forward_through_schema (in AnnotationFinder, replaces the walk-back). Advances from the schema header while every successor matches schema_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). Skips require / require_relative / load and de-duplicates class entries by name so each class only contributes its declaration line. Generator and ParsedFile both go through it.

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 yamat47 marked this pull request as ready for review May 6, 2026 16:09
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).
Copy link
Copy Markdown
Owner

@drwl drwl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the write up and making these changes!

@drwl drwl merged commit 6c3d825 into drwl:main May 8, 2026
26 checks passed
@yamat47 yamat47 deleted the fix/annotation-boundary-and-target branch May 9, 2026 09:45
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants