refactor: migrate function_lines_of_code rule#296
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the function_lines_of_code lint rule to use the updated analyzer APIs, introducing a dedicated FunctionLinesOfCodeRuleVisitor for AST traversal and adding comprehensive unit tests. The feedback suggests refactoring visitFunctionExpression in the new visitor to check the node before recursing, aligning its behavior with other visitor methods and improving readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the function_lines_of_code lint rule to use the updated analyzer APIs, introducing FunctionLinesOfCodeRuleVisitor for AST traversal and migrating the tests to a reflective test suite. Feedback on the changes suggests extracting _context.currentUnit into a local variable within _checkNode to eliminate redundant null-checks and improve code readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the function_lines_of_code lint rule to utilize the new RuleVisitorRegistry and RuleContext APIs, extracting the AST traversal logic into a dedicated FunctionLinesOfCodeRuleVisitor class. It also introduces a comprehensive set of unit tests for the rule. Feedback is provided regarding the rule's description, where using _code.problemMessage directly can expose raw placeholder templates (like {0}) in IDEs and documentation; a static, user-friendly description is recommended instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… excluding comments
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the function_lines_of_code lint rule to use the new analyzer API, migrating from custom lint builders to RuleContext and RuleVisitorRegistry. It introduces FunctionLinesOfCodeRuleVisitor to handle AST traversal, adds an empty() factory constructor to FunctionLinesOfCodeParameters, and includes a comprehensive suite of unit tests to verify the rule's behavior. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
I think this would be a good case for table driven tests that I proposed in Slack last week
Something along the lines of:
String testFunctionText(TestCase case) {
final comment = case.comments ? '\n// This is a single-line comment.\n' : '';
final multilineComment = case.comments ? '\n/*\n * This is a multi-line comment.\n */\n' : '';
return '''
int function() {${comment}
var i = 0;
${repeat('i++';, case.lines - 2)\n}${multilineComment}
return i;
}
''');
}
String repeat(String string, int times) => List.generate(times, (_) => string).join();
typedef TestCase = ({int lines, bool comments});
enum ExpectedResult {Pass, Fail};
Map<TestCase, ExpectedResult> testTable = {
(lines: 4, comments: true): Pass,
(lines: 3, comments: false): Pass,
...
};
for (final (case, result) in testTable) {
final text = testFunctionText(case);
switch (result) {
case Pass:
assertNoDiagnostics(text);
case Fail:
assertAutoDiagnostics('${expectLint(text)}');
}
}Some of those probably best extracted to some utils, and maybe a basic framework for table tests too, since we are likely to reuse it
There was a problem hiding this comment.
Great idea! Let me know what you think of this implementation.
Closes #251