-
Notifications
You must be signed in to change notification settings - Fork 3.1k
refactor: wrap rules with application reasons in tuple structure #6359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for continuedev failed. Why did it fail? →
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic found 6 issues across 7 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.name)).not.toContain( | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).not.toContain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
@@ -159,17 +159,17 @@ | |||
[srcPythonRule, utilsPythonRule], | |||
[utilsPythonFileContext], | |||
); | |||
expect(applicableRules.map((r) => r.name)).not.toContain("Src Python Rule"); | |||
expect(applicableRules.map((r) => r.name)).toContain("Utils Python Rule"); | |||
expect(applicableRules.map((r) => r.rule.name)).not.toContain("Src Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
expect(applicableRules.map((r) => r.name)).not.toContain("Src Python Rule"); | ||
expect(applicableRules.map((r) => r.name)).toContain("Utils Python Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).not.toContain("Src Python Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Utils Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
@@ -148,8 +148,8 @@ | |||
[srcPythonRule, utilsPythonRule], | |||
[srcPythonFileContext], | |||
); | |||
expect(applicableRules.map((r) => r.name)).toContain("Src Python Rule"); | |||
expect(applicableRules.map((r) => r.name)).not.toContain( | |||
expect(applicableRules.map((r) => r.rule.name)).toContain("Src Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.name)).toContain("Nested Folder Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Nested Folder Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
@@ -47,8 +47,8 @@ | |||
|
|||
// Both rules should apply | |||
expect(applicableRules).toHaveLength(2); | |||
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | |||
expect(applicableRules.map((r) => r.name)).toContain("Nested Folder Rule"); | |||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test now assumes that each element in applicableRules has a 'rule' property with a 'name', which is a breaking change from the previous structure. If any test setup or code under test still returns the old structure, this will cause test failures or runtime errors. Ensure all code paths producing applicableRules are updated to the new tuple/object structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic found 6 issues across 8 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai
to give specific feedback.
@@ -159,17 +159,17 @@ describe("Nested directory rules application", () => { | |||
[srcPythonRule, utilsPythonRule], | |||
[utilsPythonFileContext], | |||
); | |||
expect(applicableRules.map((r) => r.name)).not.toContain("Src Python Rule"); | |||
expect(applicableRules.map((r) => r.name)).toContain("Utils Python Rule"); | |||
expect(applicableRules.map((r) => r.rule.name)).not.toContain("Src Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.name)).not.toContain( | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).not.toContain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
@@ -148,8 +148,8 @@ describe("Nested directory rules application", () => { | |||
[srcPythonRule, utilsPythonRule], | |||
[srcPythonFileContext], | |||
); | |||
expect(applicableRules.map((r) => r.name)).toContain("Src Python Rule"); | |||
expect(applicableRules.map((r) => r.name)).not.toContain( | |||
expect(applicableRules.map((r) => r.rule.name)).toContain("Src Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.name)).toContain("Nested Folder Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Nested Folder Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
@@ -47,8 +47,8 @@ describe("Nested directory rules application", () => { | |||
|
|||
// Both rules should apply | |||
expect(applicableRules).toHaveLength(2); | |||
expect(applicableRules.map((r) => r.name)).toContain("Global Rule"); | |||
expect(applicableRules.map((r) => r.name)).toContain("Nested Folder Rule"); | |||
expect(applicableRules.map((r) => r.rule.name)).toContain("Global Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test now assumes that each element in applicableRules has a 'rule' property with a 'name', which is a breaking change from the previous structure. If any test setup or code under test still returns the old structure, this will cause test failures or runtime errors. Ensure all code paths producing applicableRules are updated to the new tuple/object structure.
expect(applicableRules.map((r) => r.name)).not.toContain("Src Python Rule"); | ||
expect(applicableRules.map((r) => r.name)).toContain("Utils Python Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).not.toContain("Src Python Rule"); | ||
expect(applicableRules.map((r) => r.rule.name)).toContain("Utils Python Rule"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: this assertion assumes applicableRules elements have a 'rule' property. If the structure is not updated everywhere, this will cause errors. Ensure all code producing applicableRules matches the new structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one quick fix
assuming we'll follow up with UI changes to actually use these values?
export type RuleApplicationReason = | ||
| "alwaysApply" | ||
| "implicitGlobal" | ||
| "globMatch" | ||
| "regexMatch" | ||
| "globAndRegexMatch" | ||
| "directoryMatch"; | ||
|
||
export interface RuleWithApplicationReason { | ||
rule: RuleWithSource; | ||
reason: RuleApplicationReason; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export type RuleApplicationReason = | |
| "alwaysApply" | |
| "implicitGlobal" | |
| "globMatch" | |
| "regexMatch" | |
| "globAndRegexMatch" | |
| "directoryMatch"; | |
export interface RuleWithApplicationReason { | |
rule: RuleWithSource; | |
reason: RuleApplicationReason; | |
} |
We already have this defined in index.d.ts
Summary by cubic
Refactored rule application logic to wrap each applied rule with its application reason in a tuple structure, making rule usage and debugging clearer across the codebase.
RuleWithApplicationReason
type to pair each rule with its reason for application.