feat: provide catch-all function to process dep rule#246
feat: provide catch-all function to process dep rule#246sdurnov wants to merge 2 commits intosoftarc-consulting:mainfrom
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 👍
| { | ||
| "name": "@softarc/sheriff-core", | ||
| "version": "0.19.6", | ||
| "version": "0.19.7", |
There was a problem hiding this comment.
we are going to create a separate commit for the minor release. so please revert the package.json (x2) changes.
| toTags: string[]; | ||
| } | ||
|
|
||
| // Backward compatible: supports both old signature (from, to as strings) |
There was a problem hiding this comment.
comments are not needed
| from: string; | ||
| to: string; | ||
| } & DependencyCheckContext, | ||
| context: { from: string; to: string } & Required<DependencyCheckContext>, |
There was a problem hiding this comment.
would it make sense to put from and to also in the DependencyCheckContext? I also believe we can do this without Required.
| const createMockDependencyCheckContext = ( | ||
| overrides?: Partial<DependencyCheckContext>, | ||
| ): DependencyCheckContext => ({ | ||
| fromModulePath: '' as FsPath, |
There was a problem hiding this comment.
maybe without as FsPath in each line but just one big as DependencyCheckContext at the end?
| ); | ||
|
|
||
| describe('fromTags and toTags', () => { | ||
| it('should provide fromTags in the context object', () => { |
There was a problem hiding this comment.
i don't think we need that one.
| ); | ||
| }); | ||
|
|
||
| it('should provide toTags in the context object', () => { |
There was a problem hiding this comment.
i don't think we need that one either.
| ); | ||
| }); | ||
|
|
||
| it('should allow complex cross-domain rules using fromTags and toTags', () => { |
There was a problem hiding this comment.
Can we maybe put this into a describe with the construction of config as a setup function? Each assertion could then be its own test.
I also think that this test should be enough. It is a not complicated feature with different branches which would justify that additional amount of tests.
2 participants
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