Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request expands the cognitive_complexity analysis to include Dart files within evals/test_data and specific .agents/skills/ subdirectories, rather than limiting it to the lib/ directory. Feedback suggests optimizing performance by only recursively scanning the specific target directories instead of listing the entire package directory, which can contain large build artifacts.
| if (entity is File && entity.path.endsWith('.dart') && !_isGeneratedDartFile(entity.path)) { | ||
| final String relativePath = path.relative(entity.path, from: package.directory.path); | ||
| final String posixPath = relativePath.replaceAll(r'\', '/'); | ||
| if (dir != skillsDir || posixPath.contains('/evals/test_data/')) { |
There was a problem hiding this comment.
The code before made sense because it made sure we only handled dart files and ensured that it worked cross platform. I am not following why this change need to know about evals/test_data shouldnt that filter happen in the loop?
There was a problem hiding this comment.
Refactored this so we now locate any evals/test_data directories under .agents/skills up front and append them to directoriesToSearch. The file collection loop now uses the original uniform logic without this filtering
| final String relativePath = path.relative(entity.path, from: package.directory.path); | ||
| filesToAnalyze.add(relativePath.replaceAll(r'\', '/')); | ||
| for (final dir in directoriesToSearch) { | ||
| if (!dir.existsSync()) { |
There was a problem hiding this comment.
Shouldnt the directory we configure in this tool always exist?
There was a problem hiding this comment.
I now explicitly check if the lib directory, the evals/ directory, and the .agents/skills directory exists so it's clearer.
I think in practice we know camera_android_camerax is good to go, but if folks expand this check to an arbitrary plugin, I think it would be nice to catch the edge cases.
There was a problem hiding this comment.
Are is there dart code we do not want to run cognitive_complexity on in a package? If not why not keep the existing dart file filtering and otherwise remove any directory configuration?
There was a problem hiding this comment.
Yes, test files and example apps would fail the threshold because they're large test suites. Before this change, we only analyzed lib/, excluding tests, examples, generated code, and eval test data. This PR maintains those same exclusions but adss eval test data, which requires looking outside libDirectory. Am I missing something?
camera_android_cameraxusestest_datadirectories for scripts used in evals for skills. Let's include those scripts in the cognitive complexity check. Follow up from #12369 (comment)Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2