sverweij / dependency-cruiser

Validate and visualize dependencies. Your rules. JavaScript, TypeScript, CoffeeScript. ES6, CommonJS, AMD.
https://npmjs.com/dependency-cruiser
MIT License
5.27k stars 250 forks source link

Feature request: reporters might report `comment` of rules other than `forbidden` #930

Closed jrencz closed 6 months ago

jrencz commented 6 months ago

I'm filing it as a FR not as a Bug, because I'm not aware if it's by design or it's just that no one paid attention to it yet.

Context

  1. I have configuration containing not only forbidden (as the recommended has), but also required
  2. My required rule has a comment.
  3. I have one module that violates the required rule.
  4. I execute
    depcruise src --output-type err-long

Expected Behavior

  1. I see violation reported
  2. I see a comment

Current Behavior

I do see violation reported, but I see - in place of the comment

Possible Solution

I think it's caused by the fact that when

https://github.com/sverweij/dependency-cruiser/blob/9a523b4e16ceafb9a410875fe780462b5a6c052b/src/report/error.mjs#L108-L115

looks for comment it uses findRuleByName and it

https://github.com/sverweij/dependency-cruiser/blob/9a523b4e16ceafb9a410875fe780462b5a6c052b/src/graph-utl/rule-set.mjs#L12-L16

only looks through forbidden, it doesn't take other sections.

I see that making it

-  return (pRuleSet?.forbidden ?? []).find(
+  return ([
+    ...pRuleSet?.forbidden ?? [], 
+    ...pRuleSet?.allowed ?? [], 
+    ...pRuleSet?.required ?? [],
+  ]).find(

resolves the problem - I mean: if it's not meant not to find others but forbidden. I'm happy to see it introduced as proposed (or as something that gives the same result), or I can create a PR, but if so - please let me know if feature this change touches has some tests or where can I add them

Considered alternatives

I don't think I could rephrase my required rule into a forbidden - I'm a beginner in this tool (~2h so far, but "and counting" - I definitely see its potential), but lack of ability to see comments in output reduces its usefulness for me as repository maintainer in guiding developers through rules in my repository.

sverweij commented 6 months ago

Hi @jrencz thansk for raising the issue! That's indeed a bug - the linked PR (#931) should fix it. Soon after merging it'll be released in dependency-cruiser version 16.3.1 - I'll give a heads-up in this thread when that has happened.

As you might see if you look into the PR it only adds the required rules - this is because the allowed rule (there can only be one) can't have a name.

sverweij commented 6 months ago

@jrencz the release scripts are running so it should be on npmjs in a few seconds. Let me know if it helped you!

jrencz commented 6 months ago

It did, thank you!

Thanks for fixing it during the weekend. Much appreciated! 🥇