dart-code-checker / dart-code-metrics

Software analytics tool that helps developers analyse and improve software quality.
https://dcm.dev
Other
860 stars 265 forks source link

[New rule] `forbid-import`, but allow `import ... show` #866

Closed fzyzcjy closed 2 years ago

fzyzcjy commented 2 years ago

Please describe what the rule should do:

Hi thanks for the flutter-favorite package! In my app I have a layered structure, and some layer should not see some other layer. For example, a "domain store" layer should not know how UI looks like.

Thus, I hope to forbid some kinds of imports in some files. For example, a config goes like:

dir: lib/domain_stores/**/*.dart
forbid_import:
- package:myapp/widgets/**
- package:myapp/pages/**
- package:flutter/** # because flutter package has UI things

Then, say lib/domain_stores/store_one.dart is:

import 'package:flutter/material.dart'; // LINT
import 'package:myapp/widgets/some_widget.dart'; // LINT
import 'package:myapp/other_things.dart'; // this is ok

... some code ...

If your rule is inspired by other please provide link to it:

What category of rule is this? (place an "X" next to just one item)

[ ] Warns about a potential error (problem) [ ] Suggests an alternate way of doing something (suggestion) [ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about (it will be better if you can provide both good and bad examples):

Are you willing to submit a pull request to implement this rule?


EDIT: the show logic

It would be great to allow partial imports and forbid the other part. For example, in my domain store, I do not want it to know flutter Widgets, surely, as that is a part of UI. However, I do accept that it uses MaterialColor, since "colors" is less related to UI indeed.

Thus, I hope the following could be created:

dir: lib/domain_stores/**/*.dart
forbid_import:
- path: package:flutter/material.dart
  whitelistShow: [MaterialColor, Color, ...]
- path: package:flutter/**
- path: something_else
...

Then,

import 'package:flutter/material.dart'; // LINT
import 'package:myapp/widgets/some_widget.dart'; // LINT
import 'package:myapp/other_things.dart'; // this is ok

import 'package:flutter/material.dart' show MaterialColor; // THIS IS OK AS WELL!
roman-petrov commented 2 years ago

@fzyzcjy , it seems you proposed quite similar rule I did in #820 ?

fzyzcjy commented 2 years ago

Yes, sounds similar :)

fzyzcjy commented 2 years ago

Btw I have edited the issue, and added the show logic part

roman-petrov commented 2 years ago

Btw I have edited the issue, and added the show logic part

Thanks, this is important case but in terms of architecture allowing unrelated layers to import something still might be a bad design practice, IMHO.

For example, instead of using MaterialColor in the "domain" store directly I would prefer to create some abstraction in domain store to make color manipulation and in the "ui" layer just convert my abstraction into MaterialColor.

So finally: I also have the same case in terms of layered architecture to forbid certain imports, but I am not a fan of adding whitelist for show since this still seriously violates architecture design.

fzyzcjy commented 2 years ago

Good idea! Another solution may be, create a non-blacklisted file like domain_store/colors.dart which has:

export flutter/material.dart show MaterialColor;

A bit like a hack though...

fzyzcjy commented 2 years ago

For example, instead of using MaterialColor in the "domain" store directly I would prefer to create some abstraction in domain store to make color manipulation and in the "ui" layer just convert my abstraction into MaterialColor.

That will result in a ton of boilerplate code IMHO :(

roman-petrov commented 2 years ago

All this is still subject for discussion :)

roman-petrov commented 2 years ago

@fzyzcjy , I just like to describe the reason why I do not like having whitelistShow option allowing some show imports.

In your case you would like to allow import MaterialColor class using explicit show statement.

So as I understand, if we have two modules/layers like

we will get this relationship in our architecture: ui layer uses domain layer and domain layer uses MaterailColor from ui layer (= domain layer also uses/depends on ui layer).

So we just got bi-directional relationship/dependency between our two modules. From my point of view, this is an antipattern and I don't like to have such patterns in my top-level architecture.

So, it would be great to hear other opinions here, lets invite @dkrutskikh , @incendial - can you please share your opinions on this?

fzyzcjy commented 2 years ago

@roman-petrov Agree! Indeed I have agreed before :)

So seems that the forbid-import proposal without show is sufficient, i.e. very similar to your issue.

And I would use method like creating domain_store/colors.dart which has:

export flutter/material.dart show MaterialColor;

This is because, I mentally categorize them into three layers:

Then domain only imports support layer.

roman-petrov commented 2 years ago

@roman-petrov Agree! Indeed I have agreed before :)

So seems that the forbid-import proposal without show is sufficient, i.e. very similar to your issue.

And I would use method like creating domain_store/colors.dart which has:

export flutter/material.dart show MaterialColor;

I think this aproach is just fine and it actually not gives much boilerplate code compared to whitelistShow configuration option.

BTW, the rule we are talking about will not guarantee that there will be no unwanted dependencies between some modules because you can easily can get around linter warning by export keyword, for example.

fzyzcjy commented 2 years ago

I think this aproach is just fine and it actually not gives much boilerplate code compared to whitelistShow configuration option.

Sure :)

BTW, the rule we are talking about will not guarantee that there will be no unwanted dependencies between some modules because you can easily can get around linter warning by export keyword, for example.

Agree. For that, we may need to examine every variables' type. Btw is that possible and fast enough?

incendial commented 2 years ago

Available in 4.16.0! 🚀