Fix schema_like? to recognize markdown-formatted annotation rows#354
Open
nashirox wants to merge 1 commit into
Open
Fix schema_like? to recognize markdown-formatted annotation rows#354nashirox wants to merge 1 commit into
nashirox wants to merge 1 commit into
Conversation
422cc06 to
d4c4d2b
Compare
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
Since v4.23.0, re-running annotaterb with
format_markdown: trueon a file that is already correctly annotated duplicates the entire schema annotation block instead of leaving the file unchanged / replacing it in place.Example (
Worker < User, STI model,format_markdown: true):Running annotaterb again on the already-duplicated file duplicates it a third time, and so on - it's not idempotent.
Root cause
AnnotationFinder#schema_like?(added in #334) decides where an existing annotation block ends by walking forward from its start and checking whether each subsequent line "looks like" part of the schema:This assumes at least two leading spaces after
#, which holds for the default/plain format (# id :integer not null) but not forformat_markdown: true. Markdown-formatted section headers, table rows, and list items are all rendered with a single leading space after#:# ### Columns(section header)# **`id`** | `integer` | ...(table row)# * `index_name`:(foreign key/index/enum list item)Because none of these match the two-space heuristic (and
SCHEMA_HEADER_EXACTonly matches the plain header text, not the###-prefixed markdown version),walk_forward_through_schemastops right after the very first line of the table (typically the header row).annotation_endis therefore computed far too early.When
SingleFileAnnotatorre-runs on the file,AnnotatedFile::Updater#updateonly replaces this truncated region:...leaving the rest of the old annotation (the actual column/index/foreign key rows) sitting in the file, immediately followed by the freshly generated one. Every subsequent run adds another copy.
This also affects
EnumAnnotation(# Enums) in the default format, sinceEnumAnnotation::Annotation::HEADER_TEXTwas never added toSCHEMA_HEADER_EXACT— likely an oversight from the same PR that introducedshow_enums.Fix
EnumAnnotation::Annotation::HEADER_TEXT("Enums") toSCHEMA_HEADER_EXACT, since it was missing alongside Indexes/Foreign Keys/Check Constraints.schema_like?:### <name>section headers, matched against known section names with the###prefix stripped (Columnshas noHEADER_TEXTconstant since it's inlined inAnnotation::MarkdownHeader, so it's listed explicitly).|separators (not just one) so that a one-off user comment containing a stray|isn't mistaken for a table row.* `name`:/* **`col`**), requiring a backtick right after the bullet marker so an unrelated comment starting with*isn't swallowed.Tests
annotation_finder_spec.rb: boundary detection for a markdown-formatted annotation, plus two adversarial cases confirming a trailing user comment containing a single|or a plain*bullet is not absorbed into the annotation.annotating_a_file_with_comments_spec.rb: end-to-end regression — runningSingleFileAnnotatorwithformat_markdown: trueon an already-annotated file leaves it unchanged (previously it duplicated the block).All existing specs pass (
851 examples, 0 failuresoutside ofspec/integration, which requires a full dummy Rails app/DB setup not available in this environment — verified those failures are identical with and without this change).Possible follow-up (not included in this PR)
schema_like?currently infers whether the file is markdown-formatted from the shape of each line (spacing,###,|, bullets). Since the caller (annotaterb itself) already knows the configuredformat_markdownoption atthis point, an alternative design would be to pass that flag into
AnnotationFinderand branch explicitly inschema_like?, rather than re-deriving it heuristically. That would avoid needing to keepschema_like?in sync withAnnotation::MarkdownHeader/ColumnComponent#to_markdown/ etc. every time the markdown output formatchanges. Happy to follow up with that refactor in a separate PR if maintainers are interested — keeping this PR focused on the bug fix so it's easy to review and merge quickly.