feat: provide catch-all function to process dep rule#246
Conversation
|
|
fixed stupid AI stuff and added some tests |
|
@sdurnov thanks. Plan to review it this week. |
Hi there, any quick thoughts on this maybe? |
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Good work, just a few minor comments where I think an AI should be able to handle them properly 👍
|
|
Hey, @rainerhahnekamp, hope it's fine now |
There was a problem hiding this comment.
Pull request overview
Addresses #245 by exposing the resolved tag arrays of the source and target modules to function-based dependency rule matchers. With fromTags and toTags now available in DependencyCheckContext, users can implement cross-domain / multi-dimensional dependency rules with a single catch-all ('*') function rule.
Changes:
- Add
fromTags,toTags,from, andtodirectly toDependencyCheckContextand simplifyRuleMatcherFnto receive only this context. - Populate the new tag fields when invoking
isDependencyAllowedfromcheckForDependencyRuleViolation, and propagatefrom/tovia spread insideisDependencyAllowed. - Add a
createMockDependencyCheckContexthelper and new tests demonstrating cross-domain rule patterns (shared domain, API exception, same-domain).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/lib/config/dependency-rules-config.ts | Inlines from/to and adds fromTags/toTags into the DependencyCheckContext type; simplifies RuleMatcherFn. |
| packages/core/src/lib/checks/is-dependency-allowed.ts | Spreads context first then overrides from/to when invoking the matcher function. |
| packages/core/src/lib/checks/check-for-dependency-rule-violation.ts | Supplies the new fromTags/toTags (and placeholder from/to) to isDependencyAllowed. |
| packages/core/src/lib/checks/tests/is-dependency-allowed.spec.ts | Introduces a context-mock factory and tests for cross-domain catch-all rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from: string; | ||
| to: string; |
There was a problem hiding this comment.
ah, so from and to is only needed in isDependencyAllowed but not in checkForDependencyViolation?
If that's true, we should maybe keep RuleMatcherFn as the intersection type as it was and add fromTags, toTags to DependencyCheckContext.
I think tos in isDependencyAllowed is then also not necessary anymore.
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
@sdurnov, quickly yes or no:
Should I let Copilot finish this https://github.com/softarc-consulting/sheriff/pull/246/changes#r3255612945
Either way, you will be listed as the contributor
|
Closing this in favour of a new PR that incorporates the review feedback. Full credit to @sdurnov for the original implementation — the commits are preserved as-is in the new PR. |
|
|
Great, thanks! |
|
Thank you and, we are doing a quick update of the project and then release the next version |
Here's a very drafty draft, mostly vibe coded.
rn I'm writing sheriff config for my project, and checking if it even works (seems to be working).
After I make sure, I can add few tests if necessary and fix any issues
#245