modules-graph-assert icon indicating copy to clipboard operation
modules-graph-assert copied to clipboard

Add whitelist to enable gradual refactoring

Open sherviiin opened this issue 3 years ago • 1 comments

Defining rules for a green field project is a luxury that we don't have most of the times. In reality we are 'adding' rules to existing projects and there is a high probabality that one or more modules are violating that rule. Therefore, having a temporary whitelist can help us introduce new restricting rules, so no additional violating dependency is added to project, mean while team has time to refactor the project and remove the current dependency violations.

p.s. I want to achieve this without defining alias.

sherviiin avatar Aug 17 '22 13:08 sherviiin

Hi @sherviiin,

Thank you very much for the contribution, I appreciate that. 👍

I would though not accept the PR from the following reasons -

  • There are already 2 different ways how to achieve the same thing. Use the moduleNameAlias (I read you won't want to use it - why? :) or using the allowed rules with explicitly listing the exceptions.
  • I prefer not to have another way of doing things as thing might get more confusing like that.
  • I prefer the tasks logic to be completely independent to maintain simplicity - meaning the RestrictedDependenciesAssert will now be dependent on the whitelist. This brings a question if there should be dependency on allowed, because having only whitelist providing exception and not allowed would be confusing. Answer to that question can be seen differently by different people.

What is the use case you have in mind for your change where the existing rules won't work please?

jraska avatar Aug 17 '22 15:08 jraska

Hi @jraska

Thank you for your reply. The reason I did not go with allowed rules and listing the exceptions is "too many rules, too many exceptions"! In an existing project both of those lists can be enormous, while introducing a restriction and adding whitelist for it can be done for 1 single module.

About possible confusions between whitelist & allowed (which I totally agree with), I can make it more transparent for user. I will probably do that today/tmrw.

sherviiin avatar Aug 18 '22 07:08 sherviiin

About possible confusions between whitelist & allowed (which I totally agree with), I can make it more transparent for user. I will probably do that today/tmrw.

Hey, I assume finding the time for this is tricky :)

The problem still stays though that we should possibly consider how the code would look like with using the allowed/restricted. We surely should not add new whitelist as explained above. Then comes a discussion if allowed/restricted should be even dependent on each other - I believe that not with reasons explained here and some examples below on same issue.

The other point is that this would be also breaking change in assertions behaviour and would require version 3.0 so even if we would make it work the change would go to shelf for version 3.0 🤷

Could you please share your example of concrete rules how they would look now and how they would look with allowed whitelisting the restricted rules? We can discuss them here.

jraska avatar Aug 28 '22 09:08 jraska

Hey! I ended up adding exceptions to both allowed/restricted, so it's possible to add exceptions to any of those. This helps with gradually refactor code and to adhere to newly created rule. And with that, this PR is no longer valid. Please check my fork to see if you want include that in original repository. If so I can create PR from that. Thanks.

sherviiin avatar Sep 30 '22 13:09 sherviiin