Skip to content
This repository was archived by the owner on Jul 16, 2023. It is now read-only.

feat: add initial presets #463

Closed
wants to merge 1 commit into from
Closed

feat: add initial presets #463

wants to merge 1 commit into from

Conversation

incendial
Copy link
Member

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix
[ ] New rule
[ ] Changes an existing rule
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

Let's continue the discussion about presets here with the code example.

I see several problems right now:

  • How we should handle configurable rules? Use the default config? IMO it's a tricky part since the default config generally suits no one (especially for rules like member-ordering-extended) and I'm pretty concerned about the user seeing 2000 issues just because of the inappropriate order.
  • Do we want metrics to be included into presets? This has its own pros and cons, but I generally concerned about the thresholds since they are pretty up to project/team preference.
  • Should we include dart rules into Flutter presets or it's better for the user to include these lists separately?

@roman-petrov @dkrutskikh please share your thoughts and them we should probably bring this discussion to the community too.

@incendial incendial added type: enhancement New feature or request area-core labels Sep 19, 2021
@incendial incendial added this to the 4.4.0 milestone Sep 19, 2021
@incendial incendial self-assigned this Sep 19, 2021
@codecov
Copy link

codecov bot commented Sep 19, 2021

Codecov Report

Merging #463 (814fc18) into master (6841a4d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #463   +/-   ##
=======================================
  Coverage   83.35%   83.35%           
=======================================
  Files         188      188           
  Lines        3821     3821           
=======================================
  Hits         3185     3185           
  Misses        636      636           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6841a4d...814fc18. Read the comment docs.

@roman-petrov
Copy link
Contributor

As I understand, the idea here is to introduce rule presets as yaml files, and users will have to include them into analysis_options.yaml.

The first (and most important) question for me is: do you know, is it allowed to have multiple files included into analysisis_options.yaml?

I tried using multiple include statements, but get duplicate mapping key error in VS Code.

Screenshot

image

There is a public JSON schema for pubspec.yaml, but I not see a schema for analysis_options.yaml.

I think users should be able to use dart code metrics presets together with any other rule preset package (flutter_lints, pedantic, etc). So I wonder, will this be possible using this approach?

@incendial
Copy link
Member Author

incendial commented Sep 20, 2021

@roman-petrov good point, didn't think about that. Unfortunately, the docs say Because YAML doesn’t allow duplicate keys, you can include at most one file. which means that we should rather include lints package presets into our presets or discuss with the Dart team ability to list multiple urls under include: section. Do you have any other options in mind?

@roman-petrov
Copy link
Contributor

roman-petrov commented Sep 20, 2021

Personally, I am not sure if bundling lints or flutter_lints or any other package with our presets is a good idea... I see many drawbacks here, but I am too lazy to list them :) I think this is not important at the moment.

I think it would be great to have the ability to include multiple files into analysis_options.yaml. I have some front-end (React + typescript) application development experience and I can say it is quite common to have multiple rule sets from plugins for a project.

eslint has many popular community plugins and they can be used together via extends keyword. For Dart ecosystem at the moment I see that dart code metrics seems to be almost the only analyzer plugin providing linter rules :)

So finally, I think that an almost "ideal" solution would be if we had plugin support in dart linter, multiple include support in analysis options, implement dart code metrics linter rules via dart linter plugin API and run dart code metrics via dart analyze (I am too optimistic here 😄)

Just having the ability to include multiple files into analysis options (for example, allowing a list of files to be passed into include) would be good too, but I am not sure if Dart SDK team will accept and implement such proposal.

@roman-petrov
Copy link
Contributor

Actually, I have another option in mind: we can use some "virtual" configuration settings. For example, we can provide our own "extends" section in dart code metrics configuration in analysis_options.yaml, for example:

dart_code_metrics:
   extends: 
      - dart-recommended
      - flutter-all
   rules:
      no-magic-numers: false # if user likes to disable a rule from preset

@incendial
Copy link
Member Author

Personally, I am not sure if bundling lints or flutter_lints or any other package with our presets is a good idea... I see many drawbacks here, but I am too lazy to list them :) I think this is not important at the moment.

Yeah, totally agree on that.

So finally, I think that an almost "ideal" solution would be if we had plugin support in dart linter, multiple include support in analysis options, implement dart code metrics linter rules via dart linter plugin API and run dart code metrics via dart analyze (I am too optimistic here 😄)
Just having the ability to include multiple files into analysis options (for example, allowing a list of files to be passed into include) would be good too, but I am not sure if Dart SDK team will accept and implement such proposal.

Agreed. I think I'll ask Dart SDK team first and then we can decide, what to do next.

Actually, I have another option in mind: we can use some "virtual" configuration settings. For example, we can provide our own "extends" section in dart code metrics configuration in analysis_options.yaml, for example:

I think that could be our fallback solution, but the main drawback here is about implementing merging strategy again. And since we support merging from include, we also need to be able to merge a config from 3 different places.

@incendial
Copy link
Member Author

I've posted an issue, let's see, how it goes dart-lang/sdk#47256

@roman-petrov
Copy link
Contributor

Sure, let's follow this issue for now and see if we get some progress in a reasonable amount of time.

@incendial
Copy link
Member Author

@roman-petrov looks like we should not expect the API change from the analyzer team and implement our own extend logic.

@incendial
Copy link
Member Author

How we should handle configurable rules? Use the default config? IMO it's a tricky part since the default config generally suits no one (especially for rules like member-ordering-extended) and I'm pretty concerned about the user seeing 2000 issues just because of the inappropriate order.
Do we want metrics to be included into presets? This has its own pros and cons, but I generally concerned about the thresholds since they are pretty up to project/team preference.
Should we include dart rules into Flutter presets or it's better for the user to include these lists separately?

@roman-petrov could you take a look at initial questions and share your thoughts?

@roman-petrov
Copy link
Contributor

roman-petrov commented Sep 29, 2021

@incendial, I will try to ask these questions below but definitely my answers are subject to further discussion...

How we should handle configurable rules? Use the default config? IMO it's a tricky part since the default config generally suits no one (especially for rules like member-ordering-extended) and I'm pretty concerned about the user seeing 2000 issues just because of the inappropriate order.

I agree that's quite hard to make a default configuration suit everybody. But I still think that it is good to create a default config even with rules having complex configurations. When I create a new project I like to get out-of-the-box configuration without having to define/duplicate configuration in each project. For existing projects, of course, users should be able to easily override default configuration rule settings.

Ideally, to create good default configurations, we should collect metrics from our users to find the most commonly used rule settings. Analytics is quite a big feature, so maybe we can try to find other ways to build default configurations. For example, we can analyze Flutter framework code, some Telegram votings, etc.

Also we can start with "minimum" default presets and add new rules into default presets incrementally. For example, for complex rules like member-ordering-extended we should try to find good default settings first and then think about adding them into default preset.

Do we want metrics to be included into presets? This has its own pros and cons, but I generally concerned about the thresholds since they are pretty up to project/team preference.

This is quite an interesting question. IMHO, we should not include metrics into presets for now. Or we can just create separate "rule" and "metrics" presets. I still have not integrated metrics into my project and I am quite interested in completely separating rule/metrics workflow. Maybe I will create an issue with details/proposal someday.

Should we include dart rules into Flutter presets or it's better for the user to include these lists separately?

I think we should isolate dart code metrics rule presets and not include/depend on anything external like Flutter presets. IMHO analyzer rules configuration and dart code metrics configuration should be completely separated and independent.

Sorry for a such long post, I understand that it contains too few concrete ideas, only my thoughts :) And my original idea is the same as I posted above, adding the extends section into dart code metrics configuration (pretty much the same as ESLint does):

dart_code_metrics:
   extends: 
      - dart-recommended
      - flutter-all
   rules:
      no-magic-numers: false # if user likes to disable a rule from preset
      member-ordering-extended: # if user likes to override default configuration

@incendial
Copy link
Member Author

Ideally, to create good default configurations, we should collect metrics from our users to find the most commonly used rule settings. Analytics is quite a big feature, so maybe we can try to find other ways to build default configurations. For example, we can analyze Flutter framework code, some Telegram votings, etc.

I think we won't be able to introduce any kind of analytics (which I'm pretty sad with), so our only way will be to work directly with the community.

Also we can start with "minimum" default presets and add new rules into default presets incrementally. For example, for complex rules like member-ordering-extended we should try to find good default settings first and then think about adding them into default preset.

Good point, I see it the same way.

I am quite interested in completely separating rule/metrics workflow.

We discussed it some time ago with @dkrutskikh and I think that it's a possible way of the project development.

I think we should isolate dart code metrics rule presets and not include/depend on anything external like Flutter presets.

Sorry, the question was about ours config - should we include dart_recommended into flutter_recommended, or the users will need to do it in theirs projects manually?

@incendial
Copy link
Member Author

incendial commented Oct 3, 2021

And thank you for sharing your thought, btw. It means so much!

@incendial incendial removed this from the 4.4.0 milestone Oct 3, 2021
@roman-petrov
Copy link
Contributor

roman-petrov commented Oct 19, 2021

I am quite interested in completely separating rule/metrics workflow.

We discussed it some time ago with @dkrutskikh and I think that it's a possible way of the project development.

I also started thinking on this several months ago and have some ideas, in short:

  • monorepo + melos
  • separate project into rules + metrics + core packages (not sure about actual names). Maybe, CLI should have separate package too. Maybe, we should also think about having API for programmatic metric computation.
  • introduce rules which are based on metrics like cyclomatic-complexity (similar to ESLint cyclomatic-complexity), etc.

This is not a proposal, just ideas about workflow & architecture. We can discuss this in separate issue if you think that this fits into dart code metrics roadmap.

Sorry, the question was about ours config - should we include dart_recommended into flutter_recommended, or the users will need to do it in theirs projects manually?

I think I finally got you. In my opinion, having two separate independent configurations will give better flexibility. However, if we use our "extends" implementation users will have to extend two configurations to have all recommended rules enabled. But I think it's ok: two configuration lines instead of one line is not a serious drawback, flexibility is more important.

So, finally, I think that it would be better to not include dart_recommended into flutter_recommended and have independent presets. Please let me know your thoughts on this.

@incendial incendial added this to the 4.14.0 milestone Mar 30, 2022
@incendial incendial modified the milestones: 4.14.0, 4.15.0 Apr 18, 2022
@incendial incendial modified the milestones: 4.15.0, 5.0.0 May 15, 2022
@incendial
Copy link
Member Author

Closing this, will add the implementation separately

@incendial incendial closed this Oct 9, 2022
@incendial incendial deleted the add-rules-presets branch October 9, 2022 09:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-core type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants