fix: scope lines_longer_than_80_chars suppression to files we emit#209
Merged
Conversation
Two related bugs in suppressLongLineLintInGeneratedFiles: 1. It walked the entire output directory (Directory.listSync recursive) and rewrote any .dart file with a long line. A FileRenderer subclass that overrides renderClient / renderPublicApi / etc. to no-op (so the package can hand-maintain its barrel and HTTP client) would still see those hand-written files mutated by space_gen. Concrete case: shorebird_code_push_protocol's tool/gen.dart hand-overrides the barrel, and a regen with 1.2.1 prepended a directive to the hand-written shorebird_code_push_protocol.dart. 2. It counted raw line length without the carve-outs the analyzer's lines_longer_than_80_chars lint already applies. URI imports/ exports and /// dartdoc are excluded by the lint, so a file whose only over-80 lines were imports got the directive — and then tripped unnecessary_ignore because the underlying lint never actually fired. 4 such files surfaced on the same regen. Replace the post-walk pass with a per-file emit-time helper maybeAddLongLineIgnore(content), threaded through the same _renderTemplate transform parameter that maybeAddCommentReferencesIgnore uses (the pattern PR #138 settled on for the comment_references case). Only renderApis, renderModels, renderModelTests, and _renderOneOfTest call it — the four paths that actually emit content shaped by spec-derived class names. Hand-written templates skip it; the directory-walk is gone. When measuring, the helper now skips any line whose trimmed start is 'import ', 'export ', or '///'. github regen still dart analyze clean (verified end-to-end). 530 unit tests pass. Tests: replaces the suppressLongLineLintInGeneratedFiles group with a maybeAddLongLineIgnore group that pins the new behavior — short-only, real long line, idempotency, import/export/dartdoc exclusion, mixed-content (long code beside long import still fires), borderline 80-col.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
=======================================
Coverage 94.64% 94.65%
=======================================
Files 20 20
Lines 4988 4992 +4
=======================================
+ Hits 4721 4725 +4
Misses 267 267 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Previous shape required spec-emit call sites to pick the right combination of helpers — renderApis/renderModels chained both, renderModelTests/_renderOneOfTest passed only the long-line one. That's two mechanisms to remember and an easy thing to get wrong when adding a new spec-emit path. Add maybeAddIgnoreDirectives that composes the two suppressions, and route every spec-emit call site through it. Adding or removing helpers from the chain is now a single-place edit; call sites just say transform: maybeAddIgnoreDirectives. Safe everywhere: maybeAddCommentReferencesIgnore is a no-op on content with no bracketed dartdoc tokens (e.g. round-trip test files), so applying the full chain to test renders changes nothing.
Caller intent is 'I want to render spec content' or 'I want to render a hand-written template', not 'I want this specific transform applied.' Surface the intent at the method name: - _renderTemplate: hand-written templates (pubspec, analysis_options, auth.dart, client.dart facade, etc.). No suppressions applied. - _renderSpecTemplate: model files, api files, generated round-trip tests. Auto-applies maybeAddIgnoreDirectives. Drops the optional transform: parameter from _renderTemplate. The 4 spec-emit call sites switch from passing transform: to using _renderSpecTemplate. Adding or removing helpers from the gen-time suppression chain is still a single edit in maybeAddIgnoreDirectives; adding a new spec-emit path is just a method-name choice.
This was referenced Apr 30, 2026
eseidel
added a commit
to shorebirdtech/shorebird
that referenced
this pull request
Apr 30, 2026
Pin space_gen to git ref c8937bd248495f5e32b6358dd3c5e42e62ee77d0 (eseidel/space_gen#210 tip), which fixes both regressions surfaced by the previous regen attempt: - eseidel/space_gen#209: scope long-line directive to files we emit + skip URI imports/exports when measuring (fixes the spurious directive on this package's hand-written shorebird_code_push_protocol.dart barrel and the unnecessary_ignore hits on three import-only files). - eseidel/space_gen#210: format generated content in-process at write time via DartFormatter, so the directive decision matches what the analyzer sees (fixes the orphan justification comments left after dart fix --apply stripped unnecessary directives that were emitted against pre-format content). The git pin is temporary — once the next pub.dev release ships, switch to a versioned dep. Regenerates lib/src/ from /Users/eseidel/Documents/GitHub/_shorebird/packages/code_push_server/public/openapi.yaml since api.shorebird.dev doesn't yet serve the latest spec. Verification: - dart analyze: No issues found - dart test: all 216 tests pass - 44 files changed (vs 74 in the previous regen — 30 orphan comments gone) - Hand-written barrel untouched
5 tasks
eseidel
added a commit
to shorebirdtech/shorebird
that referenced
this pull request
Apr 30, 2026
Pin space_gen to git ref a3afdf7d63a1f820a4549aca69df7af58385fc58 (eseidel/space_gen#210 tip), which fixes the regressions surfaced by the previous regen attempts: - eseidel/space_gen#209: scope long-line directive to files we emit + skip URI imports/exports when measuring (fixes the spurious directive on this package's hand-written shorebird_code_push_protocol.dart barrel and the unnecessary_ignore hits on three import-only files). - eseidel/space_gen#210 (initial commit): format generated content in-process at write time via DartFormatter, so the directive decision matches what the analyzer sees (fixes the orphan justification comments left after dart fix --apply stripped unnecessary directives that were emitted against pre-format content). - eseidel/space_gen#210 (review): in-process format now reads the consumer's language version from pubspec.lock/pubspec.yaml and the formatter:section from analysis_options.yaml (following relative include: paths so workspace setups resolve correctly). Without this, the formatter ran in tall style with default trailing-comma handling and stripped trailing commas this package's analysis_options.yaml asks to preserve. The git pin is temporary — once the next pub.dev release ships, switch to a versioned dep. Regenerates lib/src/ from /Users/eseidel/Documents/GitHub/_shorebird/packages/code_push_server/public/openapi.yaml since api.shorebird.dev doesn't yet serve the latest spec. Verification: - dart analyze: No issues found - dart test: all 216 tests pass - 36 files changed, hand-written barrel untouched, 0 orphan comments, trailing commas preserved per analysis_options.yaml
eseidel
added a commit
to shorebirdtech/shorebird
that referenced
this pull request
Apr 30, 2026
Pin space_gen to git ref 4d44b7ebb2d4b9237cd97cce2b88abb1fecd3403 (eseidel/space_gen#210 tip), which fixes a series of regressions surfaced by this regen: - eseidel/space_gen#209: scope long-line directive to files we emit + skip URI imports/exports when measuring (fixes the spurious directive on this package's hand-written shorebird_code_push_protocol.dart barrel and the unnecessary_ignore hits on three import-only files). - eseidel/space_gen#210 (initial): format generated content in-process at write time via DartFormatter, so the directive decision matches what the analyzer sees (fixes the orphan justification comments left after dart fix --apply stripped unnecessary directives that were emitted against pre-format content). - eseidel/space_gen#210 (style): in-process format reads the consumer's language version from pubspec.lock/pubspec.yaml and the formatter:section from analysis_options.yaml (following relative include: paths so workspace setups resolve correctly). Without this, the formatter ran in tall style with default trailing-comma handling and stripped trailing commas this package's analysis_options.yaml asks to preserve. - eseidel/space_gen#210 (members): comment_references resolution now also looks at field declarations, not just top-level types. Fixes the same orphan-justification problem for the comment_references directive surfaced in create_patch_artifact_request.dart, where '/// The signature of the [hash].' refers to 'final String hash;' and the analyzer correctly resolves it through the enclosing class. The git pin is temporary — once the next pub.dev release ships, switch to a versioned dep. Regenerates lib/src/ from /Users/eseidel/Documents/GitHub/_shorebird/packages/code_push_server/public/openapi.yaml since api.shorebird.dev doesn't yet serve the latest spec. Verification: - dart analyze: No issues found - dart test: all 216 tests pass - 12 files changed, hand-written barrel untouched, 0 orphan comments of either flavor, trailing commas preserved per analysis_options.yaml
eseidel
added a commit
that referenced
this pull request
Apr 30, 2026
* fix: format generated content in-process at write time #209's emit-time long-line check fired against pre-format rendered content, which had long lines that dart format later reflows under 80. Result: directive added → dart fix --apply strips it as unnecessary_ignore → 3-line justification comment orphans behind. Confirmed 61 such orphans on the shorebird codepush regen. Add dart_style as a dep and run DartFormatter in-process from _renderTemplate / _renderSpecTemplate before applying transforms. Files land on disk in canonical shape; the directive decision matches what the analyzer will actually see. Also drop the bogus carve-out for /// dartdoc lines: the lines_longer_than_80_chars lint excludes URIs (imports/exports) but DOES flag long doc comments. github regen surfaced 37 false negatives from this carve-out — all real long doc lines that needed the directive. The global formatAndFix step stays for now (dart fix still does real work like removing unused imports), but its dart format calls are now near no-ops on files we wrote — slated for removal once every emit path is on this contract. Verification: github regen dart analyze clean; 728 files now carry the directive (down from a noisier ~3000 with the post-walk), 0 orphan comment blocks, 0 false negatives on long doc comments. 530 unit tests pass. * review: detect consumer language + formatter config from output dir The previous commit's in-process format pinned to DartFormatter.latestLanguageVersion (currently 3.13.0), which forces tall style and ignores the consuming package's own analysis_options configuration. On consumers like shorebird_code_push_protocol — which specifies 'formatter: trailing_commas: preserve' in its workspace analysis_options.yaml — this strips trailing commas at write time, and the global 'dart format' that runs after can't put them back once removed. Walk up from fileWriter.outDir collecting: - pubspec.lock 'sdks.dart' lower bound (preferred — reflects the most recent 'pub get' resolution); pubspec.yaml 'environment.sdk' lower bound as a fallback before any 'pub get'. - analysis_options.yaml 'formatter:' page_width and trailing_commas. Follows a single relative-path 'include:' so workspace setups (where the package's analysis_options.yaml only inherits from the workspace root's) resolve correctly. 'package:' includes are skipped — those would need to run pub. Cache once per FileRenderer instance; the values are invariant for the lifetime of a regen. Verification: - github regen dart analyze clean. - 530 unit tests pass. * review: resolve comment_references against same-file fields too The previous emit-time helper only collected top-level type names (class / enum / mixin / extension type). A bracket ref like `[hash]` to a class field was treated as unresolved → directive added → dart fix --apply then strips it as unnecessary_ignore (the analyzer DOES resolve member refs through enclosing class scope) → 5-line justification comment orphaned. Surfaced by shorebird_code_push_protocol's create_patch_artifact_request.dart: `/// The signature of the [hash].` refers to `final String hash;` and was leaving an orphan rationale comment after every regen. Extend _topLevelDeclarations to also pick up identifiers that look like field declarations: final / late final / const / var / static followed by a type and an identifier. Two new tests pin the behavior: - ref to a same-file field is a no-op (no directive added). - placeholder refs that look like fields but have no matching declaration (the MIT license template's [year] [fullname] case) still trigger the directive. Methods/getters/setters aren't tracked — the regex would over-match too easily — so a [fooMethod] ref that resolves only via a method declaration still trips the directive. Same behavior as before; the gap is harmless (per-file directive only suppresses comment_references) and can be closed if a real-world case shows up. Verification: github regen dart analyze clean, 32 files now carry the comment_references directive (down from ~45 — member refs now correctly resolve), 0 orphan justification comments. 532 unit tests pass. * review: extract formatting concerns into formatting.dart The in-process formatter, the ignore-directive helpers, and the subprocess Formatter all lived as siblings inside file_renderer.dart. That mixed layers — FileRenderer's job is wiring templates and visitor walks, not knowing how to read pubspec.lock or compose a `// ignore_for_file:` directive. New module lib/src/render/formatting.dart holds: - longLineIgnoreBlock / commentReferencesIgnoreBlock — the per-file directive blocks. - maybeAddLongLineIgnore / maybeAddCommentReferencesIgnore / maybeAddIgnoreDirectives — the gen-time helpers wired into _renderSpecTemplate. - Formatter — the subprocess wrapper that runs dart pub get / format / fix --apply at the end of a regen. - FormatContext / FormatterConfig — the dart_style settings resolved from the consuming package's pubspec and analysis_options.yaml. - DartFileFormatter — the in-process DartFormatter wrapper with the cached resolution. Used by FileRenderer at every .dart write. FileRenderer now holds a DartFileFormatter field, calls into it from _renderTemplate / _renderSpecTemplate, and re-exports Formatter + RunProcess for the existing FileRendererConfig contract. The previously-private _FormatContext / _FormatterConfig become public so cross-file callers (and tests) can name them. Tests for the helpers move to test/render/formatting_test.dart alongside ~12 new DartFileFormatter tests covering: pass-through on non-Dart paths, formatting Dart content, pubspec.lock vs pubspec.yaml priority, formatter: page_width / trailing_commas read from analysis_options.yaml, relative include: following (workspace setup), package: include skip, walk-up to workspace root, default fallbacks, single resolution caching across many calls. Verification: - dart analyze clean. - dart test: 544 pass. - github regen dart analyze clean. * ci: format moved test files; allowlist 'mispredicting'
eseidel
added a commit
to shorebirdtech/shorebird
that referenced
this pull request
Apr 30, 2026
Bumps space_gen from a git pin to the released ^1.2.2 on pub.dev, which carries fixes surfaced by this regen (eseidel/space_gen#209 + #210): - Long-line directive scoped to files we emit, with URI imports/ exports excluded so files whose only over-80 lines are imports don't trip unnecessary_ignore. - In-process formatting at write time via DartFormatter, with per-consumer settings resolved from pubspec.lock/pubspec.yaml (language version) and analysis_options.yaml (page_width, trailing_commas — including following relative include: paths so this package's workspace setup resolves correctly). - comment_references resolution also looks at same-file field declarations, so '/// The signature of the [hash].' (referring to 'final String hash;') doesn't pick up an orphan justification comment from dart fix --apply stripping the unnecessary directive. Regenerates lib/src/ from the public spec at https://api.shorebird.dev/openapi.json (per README). Verification: - dart analyze: No issues found - dart test: all 216 tests pass - Hand-written shorebird_code_push_protocol.dart barrel untouched - Trailing commas preserved per analysis_options.yaml
eseidel
added a commit
to shorebirdtech/shorebird
that referenced
this pull request
Apr 30, 2026
Bumps space_gen from a git pin to the released ^1.2.2 on pub.dev, which carries fixes surfaced by this regen (eseidel/space_gen#209 + #210): - Long-line directive scoped to files we emit, with URI imports/ exports excluded so files whose only over-80 lines are imports don't trip unnecessary_ignore. - In-process formatting at write time via DartFormatter, with per-consumer settings resolved from pubspec.lock/pubspec.yaml (language version) and analysis_options.yaml (page_width, trailing_commas — including following relative include: paths so this package's workspace setup resolves correctly). - comment_references resolution also looks at same-file field declarations, so '/// The signature of the [hash].' (referring to 'final String hash;') doesn't pick up an orphan justification comment from dart fix --apply stripping the unnecessary directive. Regenerates lib/src/ from the public spec at https://api.shorebird.dev/openapi.json (per README). Verification: - dart analyze: No issues found - dart test: all 216 tests pass - Hand-written shorebird_code_push_protocol.dart barrel untouched - Trailing commas preserved per analysis_options.yaml
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
1 participant
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.
Summary
Two related bugs in
suppressLongLineLintInGeneratedFiles:Wrong scope. The pass walked the entire output directory and rewrote any
.dartfile with a long line. AFileRenderersubclass that overridesrenderClient/renderPublicApi/ etc. to no-op (so the package can hand-maintain its barrel and HTTP client) still saw those hand-written files mutated by space_gen. Surfaced when regeneratingshorebird_code_push_protocol: itstool/gen.darthand-overrides the barrel, and the 1.2.1 regen prepended a directive to the hand-writtenshorebird_code_push_protocol.dart.Over-trigger. The pass counted raw line length without the carve-outs the analyzer's
lines_longer_than_80_charslint applies — URIimport/exportand///dartdoc are excluded by the lint. A file whose only over-80 lines were imports got the directive anyway, then trippedunnecessary_ignorebecause the underlying lint never fires. 4 such files showed up in the same regen.Fix
Mirrors the pattern PR #138 settled on for
comment_references: replace the post-walk pass with a per-file emit-time helpermaybeAddLongLineIgnore(content), threaded through the same_renderTemplatetransform parameter. Only the four paths that emit content shaped by spec-derived class names call it:renderApis,renderModels,renderModelTests,_renderOneOfTest. Hand-written templates skip it.When measuring, the helper skips any line whose trimmed start is
import,export, or///— matching the analyzer's own carve-outs.The post-walk function is gone; the call site in
render()is gone.Verification
dart analyzeclean end-to-end.shorebird_code_push_protocolregen that exposed both bugs: when re-pinned to a future release with this fix, the hand-written barrel stays untouched and the 4 false-positive directives don't appear.Test plan
suppressLongLineLintInGeneratedFilestest group with amaybeAddLongLineIgnoregroup covering: short-only content, real long line, idempotency, import-only / export-only / dartdoc-only long-lines (no directive), mixed-content (long code next to long import still fires), borderline 80-col.dart analyzeclean (/tmp/g_post_refactor2).