Feature: Custom code gen class name #694
Conversation
|
@aissat Hi, could you please review this PR when you have a chance? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
WalkthroughAdds a new CLI option (--class-name / -c) wired into GenerateOptions.className, validates it at runtime for 'keys' format, and uses it when generating the keys file so the produced header is Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as generate.dart CLI
participant Gen as Generator
participant Writer as _writeKeys
User->>CLI: run (optionally --class-name MyKeys)
CLI->>Gen: create GenerateOptions(className: "MyKeys"|null)
Gen->>Gen: compute effective className (trim/fallback), validate if format=='keys'
Gen->>Writer: _writeKeys(className)
Writer-->>Gen: writes file with "abstract class <className> { ... }"
Gen-->>User: output files generated / error on invalid name
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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 (4)
bin/generate.dart (4)
84-91: Drop null-assertion in callback and clarify help textAvoid
x!— it’s unnecessary (you already have a fallback later) and brittle. Also, clarify that the option affects the keys format only.Apply this diff:
parser.addOption( 'class-name', abbr: 'c', defaultsTo: 'LocaleKeys', - callback: (String? x) => generateOptions!.className = x!, - help: 'Custom class name for generated file', + callback: (String? x) => generateOptions!.className = x, + help: 'Custom class name for generated keys class (keys format)', );If you’d like, I can also update the README/usage docs to mention the new flag and its scope (keys format only). Want me to open a docs PR?
102-102: IncludeclassNameinGenerateOptions.toString()for easier debuggingPrinting the parsed
classNamehelps when troubleshooting CLI invocations.Outside the selected lines, update
toString()like this:@override String toString() { - return 'format: $format sourceDir: $sourceDir sourceFile: $sourceFile outputDir: $outputDir outputFile: $outputFile skipUnnecessaryKeys: $skipUnnecessaryKeys'; + return 'format: $format ' + 'sourceDir: $sourceDir ' + 'sourceFile: $sourceFile ' + 'outputDir: $outputDir ' + 'outputFile: $outputFile ' + 'skipUnnecessaryKeys: $skipUnnecessaryKeys ' + 'className: $className'; }
152-158: Validate--class-nameto ensure a valid Dart identifierGuard against invalid class names (e.g., names starting with a digit or containing spaces) to avoid generating uncompilable code. Trim whitespace and validate only when format is
keys.Apply this diff:
- final className = options.className ?? 'LocaleKeys'; + final className = (options.className?.trim().isNotEmpty ?? false) + ? options.className!.trim() + : 'LocaleKeys'; + if (options.format == 'keys' && + !RegExp(r'^[A-Za-z_$][A-Za-z0-9_$]*$').hasMatch(className)) { + stderr.writeln( + "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _; only letters, digits, _ or \$)."); + return; + }
186-198: Fix double space in generated class declarationMinor formatting nit: there’s a double space between
classand$className.Apply this diff:
-abstract class $className { +abstract class $className {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bin/generate.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
bin/generate.dart (1)
170-172: ThreadingclassNamethrough to_writeKeyslooks goodPassing the resolved class name into the writer is the right place to apply the option.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bin/generate.dart (3)
84-91: New --class-name flag looks good; consider trimming input and clarifying help text.Minor UX polish: trim the provided value early and make the help text explicit about constraints and default.
parser.addOption( 'class-name', abbr: 'c', defaultsTo: 'LocaleKeys', - callback: (String? x) => generateOptions!.className = x, - help: 'Custom class name for generated keys class (keys format)', + callback: (String? x) => generateOptions!.className = x?.trim(), + help: 'Custom class name for the generated keys class (used only with --format keys). Must be a valid Dart identifier. Default: LocaleKeys', );
158-162: Prefer returning Future for async API surface (avoid async void).
async voidswallows errors and can’t be awaited. UsingFuture<void>improves error propagation and composability. If you take this, alsoawaitthe call site inhandleLangFiles.-void generateFile( +Future<void> generateFile( List<FileSystemEntity> files, Directory outputPath, GenerateOptions options, ) async {Outside this hunk, in
handleLangFiles(...), update the call:await generateFile(files, outputPath, options);And consider changing
handleLangFilessignature toFuture<void>as well for consistency.
211-211: Dynamic class header emission works; consider a gentle style nudge.Given this class is meant to be imported, a leading underscore would make it library-private. You might optionally warn if
className.startsWith('_'), and/or recommend PascalCase in the help text (already covered above).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
bin/generate.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
bin/generate.dart (4)
102-102: Good addition to GenerateOptions.Adding
classNameto options is consistent with other fields and keeps it discoverable in one place.
107-114: toString update is fine.Including
classNamein the debugging output is helpful for tracing CLI usage.
184-186: Threading the className into _writeKeys is correct.This keeps the generator pure and makes the output deterministic from options.
200-206: _writeKeys signature change is appropriate.Passing
classNameexplicitly improves testability and avoids hidden globals.
| final className = (options.className?.trim().isNotEmpty ?? false) | ||
| ? options.className!.trim() | ||
| : 'LocaleKeys'; | ||
| if (options.format == 'keys' && | ||
| !RegExp(r'^[A-Za-z_$][A-Za-z0-9_$]*$').hasMatch(className)) { | ||
| stderr.writeln( | ||
| "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _; only letters, digits, _ or \$)."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Fix null-aware chaining bug and tighten Dart identifier validation (disallow '$', check reserved words).
options.className?.trim().isNotEmptyis a static error:trim()is null-aware, yieldingString?, so.isNotEmptymust also be null-aware.- The identifier regex currently allows
$, which is not valid in Dart identifiers. - Optional: Guard against reserved words to prevent generating uncompilable code.
- final className = (options.className?.trim().isNotEmpty ?? false)
- ? options.className!.trim()
- : 'LocaleKeys';
- if (options.format == 'keys' &&
- !RegExp(r'^[A-Za-z_$][A-Za-z0-9_$]*$').hasMatch(className)) {
- stderr.writeln(
- "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _; only letters, digits, _ or \$).");
- return;
- }
+ final rawClassName = options.className?.trim();
+ final className =
+ (rawClassName?.isNotEmpty ?? false) ? rawClassName! : 'LocaleKeys';
+ if (options.format == 'keys') {
+ // Valid Dart identifier: letters or _ to start; then letters/digits/_ only. No '$'.
+ final isValidIdentifier =
+ RegExp(r'^[A-Za-z_][A-Za-z0-9_]*$').hasMatch(className);
+ const reservedWords = {
+ 'abstract','as','assert','async','await','break','case','catch','class','const',
+ 'continue','covariant','default','deferred','do','dynamic','else','enum','export',
+ 'extends','extension','external','factory','false','final','finally','for','Function',
+ 'get','hide','if','implements','import','in','interface','is','late','library','mixin',
+ 'new','null','on','operator','part','required','rethrow','return','set','show','static',
+ 'super','switch','sync','this','throw','true','try','typedef','var','void','while','with',
+ 'yield'
+ };
+ if (!isValidIdentifier || reservedWords.contains(className)) {
+ stderr.writeln(
+ "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _, contain only letters, digits, or _), and not be a reserved word.");
+ return;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final className = (options.className?.trim().isNotEmpty ?? false) | |
| ? options.className!.trim() | |
| : 'LocaleKeys'; | |
| if (options.format == 'keys' && | |
| !RegExp(r'^[A-Za-z_$][A-Za-z0-9_$]*$').hasMatch(className)) { | |
| stderr.writeln( | |
| "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _; only letters, digits, _ or \$)."); | |
| return; | |
| } | |
| final rawClassName = options.className?.trim(); | |
| final className = | |
| (rawClassName?.isNotEmpty ?? false) ? rawClassName! : 'LocaleKeys'; | |
| if (options.format == 'keys') { | |
| // Valid Dart identifier: letters or _ to start; then letters/digits/_ only. No '$'. | |
| final isValidIdentifier = | |
| RegExp(r'^[A-Za-z_][A-Za-z0-9_]*$').hasMatch(className); | |
| const reservedWords = { | |
| 'abstract','as','assert','async','await','break','case','catch','class','const', | |
| 'continue','covariant','default','deferred','do','dynamic','else','enum','export', | |
| 'extends','extension','external','factory','false','final','finally','for','Function', | |
| 'get','hide','if','implements','import','in','interface','is','late','library','mixin', | |
| 'new','null','on','operator','part','required','rethrow','return','set','show','static', | |
| 'super','switch','sync','this','throw','true','try','typedef','var','void','while','with', | |
| 'yield' | |
| }; | |
| if (!isValidIdentifier || reservedWords.contains(className)) { | |
| stderr.writeln( | |
| "Invalid --class-name '$className'. It must be a valid Dart identifier (start with a letter or _, contain only letters, digits, or _), and not be a reserved word."); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In bin/generate.dart around lines 163 to 171, fix the null-aware chaining and
tighten identifier validation: ensure you call trim() only when className is
non-null and propagate null-safety when checking isNotEmpty (e.g., use
options.className != null && options.className!.trim().isNotEmpty or store
trimmed value in a local non-null variable before checking), change the RegExp
to disallow '$' (use something like r'^[A-Za-z_][A-Za-z0-9_]*$' so identifiers
start with a letter or underscore and contain only letters, digits or
underscores), and add a reserved-word guard (compare the candidate className
against a set of Dart reserved words and reject or alter it) so the tool never
emits an invalid Dart identifier; update the stderr message accordingly.
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Hi, this is needed when we import localizations from multiple packages.
#449
Summary by CodeRabbit
New Features
Bug Fixes / Validation