dart-code-metrics icon indicating copy to clipboard operation
dart-code-metrics copied to clipboard

feat: add initial presets

Open incendial opened this issue 4 years ago • 14 comments

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 avatar Sep 19 '21 09:09 incendial

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.

codecov[bot] avatar Sep 19 '21 09:09 codecov[bot]

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?

roman-petrov avatar Sep 20 '21 06:09 roman-petrov

@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?

incendial avatar Sep 20 '21 07:09 incendial

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 avatar Sep 20 '21 08:09 roman-petrov

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

roman-petrov avatar Sep 20 '21 08:09 roman-petrov

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 avatar Sep 20 '21 14:09 incendial

I've posted an issue, let's see, how it goes https://github.com/dart-lang/sdk/issues/47256

incendial avatar Sep 20 '21 15:09 incendial

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

roman-petrov avatar Sep 21 '21 06:09 roman-petrov

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

incendial avatar Sep 28 '21 18:09 incendial

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?

incendial avatar Sep 28 '21 18:09 incendial

@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

roman-petrov avatar Sep 29 '21 07:09 roman-petrov

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 avatar Oct 03 '21 08:10 incendial

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

incendial avatar Oct 03 '21 08:10 incendial

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.

roman-petrov avatar Oct 19 '21 14:10 roman-petrov

Closing this, will add the implementation separately

incendial avatar Oct 09 '22 08:10 incendial