-
Notifications
You must be signed in to change notification settings - Fork 3.1k
RulesContextProvider #6070
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
RulesContextProvider #6070
Conversation
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev canceled.
|
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.
LGTM, few nitpick comments, only other thing is I think we should use the PencilIcon
instead of the current default ChatBubbleIcon
so that we mirror what is in the notch.
@@ -39,6 +41,24 @@ import { migrateJsonSharedConfig } from "../migrateSharedConfig"; | |||
import { rectifySelectedModelsFromGlobalContext } from "../selectedModels"; | |||
import { loadContinueConfigFromYaml } from "../yaml/loadYaml"; | |||
|
|||
async function loadRules(ide: IDE) { |
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.
This is going to conflict with @tomasz-stefaniak PR, I think this is a good improvement though because his PR adds a 3rd rule source that we load from
return rule.ruleFile ?? rule.slug ?? rule.name ?? rule.rule; | ||
} | ||
|
||
private getNameFromRule(rule: RuleWithSource): string { |
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.
We do this in convertMarkdownRuleToContinueRule
to grab a fallback name from the first H1 tag in the markdown.
// Try to extract title from first heading if no name in frontmatter
let name = frontmatter.name;
if (!name) {
// Look for a heading in the markdown
const headingMatch = markdown.match(/^#\s+(.+)$/m);
if (headingMatch) {
name = headingMatch[1].trim();
} else {
// Fall back to filename
name = basename(path).replace(/\.md$/, "");
}
}
Kind of edge case though when we have the name
property. If you don't think it's worth pulling in here maybe we just get rid of that logic in convertMarkdownRuleToContinueRule
to keep behavior consistent though.
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.
I think the difference between this and convertMarkdownRuleToContinueRule
is that RuleWithSource
doesn't need to have a name. That function actually should get called prior to this one, so I don't think this method will ever be used. But is necessary for typing because I don't think we'd want to make name
required on RuleWithSource
(it wouldn't match the .md source). The name
, id
, and description
are really concepts that are unique to the ContextItem
interface so I don't think they would need to be used anywhere else
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.
A lot of the logic here feels like it should live in loadMarkdownRules.ts
or some other rules
subfolder so that we can reuse it, which I anticipate happening. E.g. most of @tomasz-stefaniak 's new work lives in core/config/markdown
Also would make it easier to test some of these helper fns
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.
for the reason mentioned in my other comment I actually kind of think that these small methods won't ever need to be used anywhere else
Description
Introduces the RulesContextProvider, which is available by default
Checklist
Tests
I don't feel that any meaningful logic was introduced that needs to be tested. If we tested some of the private methods on the class, that feels like it would be overkill