Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reconsider how we configure lint rules in analysis options objects #54045

Closed
Tracked by #53876
pq opened this issue Nov 14, 2023 · 3 comments
Closed
Tracked by #53876

Reconsider how we configure lint rules in analysis options objects #54045

pq opened this issue Nov 14, 2023 · 3 comments
Labels
analyzer-analysis-options analyzer-linter Issues with the analyzer's support for the linter package analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@pq
Copy link
Member

pq commented Nov 14, 2023

In https://dart-review.googlesource.com/c/sdk/+/335063/3/pkg/linter/test/rule_test.dart, @scheglov notes:

I understand that we need to configure lint rules in some way, but I'm concerned about the current approach of modifying the analysis options object directly. This can lead to code that is difficult to maintain and understand.

I would propose an alternative approach that involves using or generating an analysis_options.yaml file. This would allow us to define lint rules in a more declarative and maintainable way, and it would also align with how the analysis server handles lint rules.

I believe that this change would make our code more readable, easier to understand, and less prone to errors. I would be happy to discuss this further if you have any questions.

@pq pq added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-linter Issues with the analyzer's support for the linter package P2 A bug or feature request we're likely to work on analyzer-technical-debt analyzer-analysis-options labels Nov 14, 2023
@scheglov
Copy link
Contributor

Let me re-state it a bit easier, without LLM nonsense.

I hoped to be able to use AnalysisContextCollection[Impl] for all our analysis uses.
And I did a big push to use it for the analysis server itself.
Just give it the correctly configured file system, and let the analyzer do its work.
Don't go to the low level, and don't patch internal data.
This should be the last resort, if we really cannot do otherwise.

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Mar 28, 2024
@srawlins
Copy link
Member

I made a big step in this direction with

However, a few fields remain mutable. In particular enabledLegacyPluginNames is still writable and is still modified in tests.

@srawlins
Copy link
Member

And with https://dart-review.googlesource.com/c/sdk/+/392240 I think this can be closed. We still pass around this annoying updateAnalysisOptions argument around various functions, like AnalysisContextCollectionImpl. We do not update lint rules with that function; it's usually warning, lint, and contextFeatures. Please re-open if there are more specific actions to take here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-analysis-options analyzer-linter Issues with the analyzer's support for the linter package analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants