⚡️ Stop writing // dart format width=80 to the generated freezed files#1256
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new Dart code formatting step was integrated into the Freezed code generation process, using the Changes
Sequence Diagram(s)sequenceDiagram
participant Builder
participant DartFormatter
participant FileSystem
Builder->>DartFormatter: Format generated Dart code
DartFormatter-->>Builder: Return formatted code
Builder->>FileSystem: Write formatted code to .freezed.dart file
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/freezed/lib/builder.dart (1)
30-31: Consider adding error handling to the formatter function.The
_defaultFormatOutputfunction correctly implements the required signature for source_gen's formatOutput parameter. However, consider adding error handling sinceDartFormatter.format()could potentially throw exceptions on malformed code.-String _defaultFormatOutput(String code, Version version) => - DartFormatter(languageVersion: version).format(code); +String _defaultFormatOutput(String code, Version version) { + try { + return DartFormatter(languageVersion: version).format(code); + } catch (e) { + // Fall back to unformatted code if formatting fails + return code; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/freezed/CHANGELOG.md(1 hunks)packages/freezed/lib/builder.dart(3 hunks)packages/freezed/pubspec.yaml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: freezed (packages/freezed, master, get)
🔇 Additional comments (4)
packages/freezed/CHANGELOG.md (1)
1-4: Documentation accurately reflects the change.The changelog entry clearly documents the removal of the
// dart format width=80comment from generated files, which aligns with the PR objectives and the implementation changes in the other files.packages/freezed/pubspec.yaml (1)
16-16:Details
✅ Verification successful
Verify dependency versions for security and currency.
The addition of
dart_styleandpub_semverdependencies is necessary for the new formatting functionality. However, please verify that these versions are current and free from known security vulnerabilities.Also applies to: 22-22
🏁 Script executed:
#!/bin/bash # Description: Check for latest versions and security advisories for the new dependencies echo "Checking dart_style latest version:" curl -s https://pub.dev/api/packages/dart_style | jq '.latest.version' echo "Checking pub_semver latest version:" curl -s https://pub.dev/api/packages/pub_semver | jq '.latest.version' echo "Checking for security advisories:" gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: PUB) { nodes { advisory { summary severity publishedAt } package { name } vulnerableVersionRange firstPatchedVersion { identifier } } } }' | jq '.data.securityVulnerabilities.nodes[] | select(.package.name == "dart_style" or .package.name == "pub_semver")'Length of output: 823
Dependencies up-to-date and secure.
dart_styleconstraint^3.0.0already allows the latest 3.1.0 release.pub_semverpinned at^2.2.0matches the latest 2.2.0.- No known security advisories were found for either package in the PUB ecosystem.
No further changes needed. Approving these dependency additions.
packages/freezed/lib/builder.dart (2)
2-2: Good use of specific imports.Using
showto import only the specific classes needed (DartFormatterandVersion) is a good practice that keeps the namespace clean and makes dependencies explicit.Also applies to: 4-4
19-19: Correct implementation of formatOutput override.Adding the
formatOutput: _defaultFormatOutputparameter toPartBuilderis the correct approach to override source_gen's default behavior and prevent the// dart format width=80comment from being added to generated files.
|
Some tests are necesary here :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/freezed/test/builder_test.dart (1)
64-104: Consider clarifying the purpose of the explicit format option.This test uses
{'format': true}while the previous test uses empty options{}. If both scenarios should behave identically regarding the dart format comment removal, consider adding a comment explaining why both configurations are tested.await testBuilder( - freezed(const BuilderOptions({'format': true})), + freezed(const BuilderOptions({'format': true})), // Test explicit format option
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/freezed/lib/builder.dart(3 hunks)packages/freezed/test/builder_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/freezed/lib/builder.dart
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: freezed (packages/freezed, master, get)
- GitHub Check: freezed (packages/freezed/example, master, get)
🔇 Additional comments (6)
packages/freezed/test/builder_test.dart (6)
1-8: LGTM! Clean and appropriate imports.The imports are well-organized and include all necessary dependencies for testing the formatting functionality.
18-21: Good test of direct formatting function.The test correctly verifies that the
defaultFormatOutputfunction formats code properly while avoiding the dart format width comment. The use ofVersion(3, 0, 0)for testing is appropriate.
29-62: Excellent integration test coverage.This test thoroughly validates the core PR objective by:
- Testing the full builder pipeline with realistic freezed code
- Verifying absence of
// dart format width=comments- Confirming presence of expected generated file headers
The test structure using
InMemoryAssetWriterandtestBuilderis appropriate for build system testing.
95-95: Consistent assertion pattern.Good use of
contains('// dart format')instead of the more specificcontains('// dart format width=')used in other tests. This broader check ensures no dart format comments are present regardless of the specific variant.
10-27:Details
✅ Verification successful
Verify that
defaultFormatOutputis exported from the builder module.The test directly imports and uses
defaultFormatOutputfrompackage:freezed/builder.dart. Ensure this function is properly exported in the public API.
🏁 Script executed:
#!/bin/bash # Description: Verify that defaultFormatOutput is exported from the builder module # Expected: Function should be found in the builder.dart file exports # Check if defaultFormatOutput is exported from builder.dart rg -A 5 "defaultFormatOutput" packages/freezed/lib/builder.dartLength of output: 672
No action needed:
defaultFormatOutputis already exported frombuilder.dart.
The function is defined and available inpackages/freezed/lib/builder.dart, so the test import is valid.
101-103:Details
❓ Verification inconclusive
Verify the expected mixin and getter patterns.
The test asserts specific code structure patterns. Ensure these assertions align with the actual output format of the freezed generator.
🏁 Script executed:
#!/bin/bash # Description: Verify the expected patterns in generated freezed files match test assertions # Expected: Generated files should contain the patterns being tested # Check for similar patterns in existing freezed generated files or examples fd "\.freezed\.dart$" --exec rg -l "mixin.*\$.*\{" {} \; fd "\.freezed\.dart$" --exec rg -l "get.*;" {} \; | head -5Length of output: 272
🏁 Script executed:
#!/bin/bash # Inspect mixin and getters indentation in generated freezed files for file in benchmarks/lib/src/*.freezed.dart; do echo "=== $file ===" # Find the first mixin declaration line=$(grep -n "mixin _\$" "$file" | cut -d: -f1 | head -n1) if [ -z "$line" ]; then echo "No mixin found" continue fi # Print the mixin line and the next 5 lines to capture getters sed -n "${line},$((line+5))p" "$file" echo doneLength of output: 631
🏁 Script executed:
#!/bin/bash # Locate and display the test file with context around the assertions file=$(fd builder_test.dart) echo "Found test file: $file" echo "Displaying lines 80-120:" sed -n '80,120p' "$file"Length of output: 1146
🏁 Script executed:
#!/bin/bash # Display the first 30 lines of the example generated file to inspect mixin and getters file="packages/freezed_lint/example/lib/missing_mixin.freezed.dart" if [ -f "$file" ]; then echo "=== $file ===" sed -n '1,30p' "$file" else echo "File not found: $file" fiLength of output: 1833
Verify Freezed Output for Mixin and Getter Syntax
Please confirm that the generated
simple_class.freezed.dartfile matches the test’s expectations:
- It declares the mixin exactly as
mixin _$SimpleClass {- The getters are indented two spaces and terminated with semicolons:
String get name;
int get age;Affected test: packages/freezed/test/builder_test.dart lines 101–103.
|
@rrousselGit PTAL |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/freezed/test/source_gen_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: freezed (packages/freezed/example, master, get)
- GitHub Check: freezed (packages/freezed, master, get)
- GitHub Check: freezed (packages/freezed_lint, master, get)
- GitHub Check: freezed (packages/freezed_annotation, master, get)
🔇 Additional comments (1)
packages/freezed/test/source_gen_test.dart (1)
2-2: LGTM - necessary imports for the new test.The added imports are appropriate for the file system operations and test framework usage in the new test case.
Also applies to: 7-7
|
@rrousselGit Would you take another look? |
|
LGTM |
2 participants
The builder of freezed does not specify the
formatOutput, which will produce the defaultwidth=80header in generated files. #1160 removes the handling, so the default implementation is being used. The PR prevents it from being generated.See also:
Where it got inserted:
How
json_serializablehandles it:Summary by CodeRabbit
New Features
Bug Fixes
// dart format width=80comment.Tests
Documentation
Chores