Implementation of parameter parser#234
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the AnalysisOptionsLoader and LintOptions classes to parse and manage lint configurations from analysis_options.yaml. The review identifies critical issues with the current caching strategy, noting that a global cache will cause incorrect behavior in monorepos and lacks a mechanism for invalidation when configuration files change. Further recommendations include renaming variables for clarity and consistently using the analyzer's ResourceProvider API instead of dart:io to ensure better compatibility and testability.
|
|
||
| /// Loads and parses analysis options from a Dart project's YAML file. | ||
| class AnalysisOptionsLoader { | ||
| Map<String, LintOptions> _rulesCache = {}; |
There was a problem hiding this comment.
The current caching mechanism does not support cache invalidation. In a long-running Analysis Server session (e.g., in an IDE), changes to analysis_options.yaml will not be detected, and the plugin will continue using stale configuration until the server is restarted. Consider checking the file's modification timestamp or implementing a file watcher to refresh the cache when the configuration changes.
There was a problem hiding this comment.
@Dariaa14, have you investigated the problem described here? Looks pretty relevant overall, but I'm not sure if changing analysis_options.yaml would restart the analysis server and reload the configuration.
solid-vovabeloded
left a comment
solid-vovabeloded
left a comment
There was a problem hiding this comment.
Great start, @Dariaa14! Please, take a look at the following suggestions and let me know what you think.
|
|
||
| /// Loads and parses analysis options from a Dart project's YAML file. | ||
| class AnalysisOptionsLoader { | ||
| Map<String, LintOptions> _rulesCache = {}; |
There was a problem hiding this comment.
@Dariaa14, have you investigated the problem described here? Looks pretty relevant overall, but I'm not sure if changing analysis_options.yaml would restart the analysis server and reload the configuration.
| _analysisLoader.loadRulesFromContext(context); | ||
| // To get the options of the rule: | ||
| // _analysisLoader.getRuleOptions(context, lintName); |
There was a problem hiding this comment.
I know this is just a usage example - leaving this comment for the future. We would need to create a separate class for our rules that would encapsulate this logic and just provide an API to get the rule settings.
style: improve variable names refactor: use root package path instead of library path refactor: use pattern matching to reduce nesting
refactor: allow mocking resource provider refactor: extract CachedPackageRules model
refactor: use Map<String, Object?> instead of LintOptions as the enabled field is implicitly true for all rules that the analyzer processes remove RuleConfig as it is no longer needed
3 participants
Implementation:
Since the analysis_server_plugin currently lacks a direct way to retrieve the context root (noted by the TODO in the official source code), I explored two strategies for resolving analysis_options.yaml:
Plugin Register: I found this too restrictive, as it only functions correctly when 'dart analyze' is executed from the project root.
Rule Registration with Caching (Chosen): By resolving the configuration at the rule level, we can leverage the RuleContext to identify the file path currently under analysis. To prevent the performance overhead of re-parsing for every file, I implemented a caching layer. The YAML is only parsed once per 'dart analyze'; subsequent files retrieve the configuration from the cache.
Research of other packages:
I researched the approach of others:
custom_lint and dart_code_metrics both utilize a custom server architecture built on the legacy analyzer_plugin package.
I found no existing public implementations for dynamic parameter injection for analysis_server_plugin users.
Configuration Structure
Regarding our previous discussion on YAML structure: I tested the older formats used by other analyzers, but they were not recognized by the current server plugin. I have reverted to the structure specified in the official Dart documentation, which I have verified as functional in this implementation.