feat: generate rules files when configuring the MCP#46
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
nedaKaighobadi
changed the title
src/commands/configureMCP.ts
Outdated
| fs.writeFileSync(filePath, JSON.stringify(modifiedConfig, null, 2)) | ||
|
|
||
| vscode.window.showInformationMessage('Codacy MCP server added successfully') | ||
| createRules() |
There was a problem hiding this comment.
The asynchronous function createRules() is called without awaiting its result in configureMCP. Consider using 'await createRules()' to ensure proper sequential execution and error handling.
| createRules() | |
| await createRules() |
There was a problem hiding this comment.
wow, copilot found a good one!
| fs.writeFileSync(rulesPath, `${existingContent}\n${convertRulesToMarkdown(newRules, existingContent)}`) | ||
| } | ||
|
|
||
| vscode.window.showInformationMessage(`Updated rules in ${rulesPath}`) |
There was a problem hiding this comment.
I was wondering if this type of work should be visible to the user or not. Might be a lot of popup appearing. They will have "MCP Setup successfully" and right after something about rules... Maybe we don't need to make this a popup, just add them as Logs? What do you think?
There was a problem hiding this comment.
I thought about it being too many popups as well. But at the same time, I think it's better this way since we are modifying a file (two files actually if we count the gitignore) in the user's workspace and it's more transparent this way
| }) | ||
| ) | ||
|
|
||
| if (isMCPConfigured()) { |
There was a problem hiding this comment.
shall we also check the rules don't previously exist?
There was a problem hiding this comment.
I do the check when creating the rules file
There was a problem hiding this comment.
But it can be improved, I'm working on properly manipulating the text so that the rules are nicely replaced or modified if they exist with different enforcers
src/commands/configureMCP.ts
Outdated
| // Add the created file to .gitignore | ||
| const relativeRulesPath = path.relative(workspacePath, rulesPath) | ||
| const gitignoreContent = `\n\n#Ignore ${currentIDE} AI rules\n${relativeRulesPath}\n` | ||
| let existingGitignore = '' | ||
|
|
||
| if (fs.existsSync(gitignorePath)) { | ||
| existingGitignore = fs.readFileSync(gitignorePath, 'utf8') | ||
|
|
||
| if (!existingGitignore.split('\n').some((line) => line.trim() === relativeRulesPath.trim())) { | ||
| fs.appendFileSync(gitignorePath, gitignoreContent) | ||
| vscode.window.showInformationMessage(`Added ${relativeRulesPath} to .gitignore`) | ||
| } | ||
| } else { | ||
| fs.writeFileSync(gitignorePath, gitignoreContent) | ||
| vscode.window.showInformationMessage('Created .gitignore and added rules path') | ||
| } | ||
| } catch (error: unknown) { | ||
| const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred' | ||
| vscode.window.showErrorMessage(`Failed to create rules: ${errorMessage}`) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
I do have my doubts about this... i see the benefit, yes... but perhaps this should be a user decision. it's not like it has personal things, the content of the file can be used by other developers using cursor/copilot/whatever
There was a problem hiding this comment.
I see your point, but at the same time I think it's weird commiting IDE specific files and folders, I think it might end up polluting the workspace. Maybe I'm biased here and added this based on personal preference, but I don't like when I install something in the IDE (which I find to be very personalised to each developer) and it adds stuff that are commitable and then have to go and hide it manually. But I can remove this if we think it's not necessary 🤔
There was a problem hiding this comment.
it's not a blocker, so we can go either with this or without it, and then check the feedback; it's totally out of preference... so I guess that in both cases, we'll be benefiting some and giving work to others.... which group is bigger, who knows 🤷♂️
| if (parts.length < 3) { | ||
| throw new Error('Invalid MDC file format: missing frontmatter') | ||
| } |
There was a problem hiding this comment.
I'm so confused about this..... I trust it has a reason, I'd add a comment in the code making it explicit
There was a problem hiding this comment.
Ah yeah, this for the mdc format that cursor rules follow that need to have a 'header' that sets the rule as 'always applying' or if it needs to be applied manually. I will add a comment explaining this
There was a problem hiding this comment.
ok, now that you mention this, for the rule types, instead of "always applying", did you try with "agent rule"? and I'm saying this thinking about the parameters one... but perhaps the other about checking with the CLI should always be there....mmmm... I'm diverging in this comment, but perhaps in the future will be good considering different .mdc files with different scopes (not now)
src/commands/configureMCP.ts
Outdated
| }, | ||
| { | ||
| when: 'after generating code', | ||
| enforce: ['Analyze the code using cli analysis tool and then correct any issues found'], |
There was a problem hiding this comment.
should this be "using Codacy cli analysis tool" just in case users have other cli analysis tools?
There was a problem hiding this comment.
yes, I have changed it. Now it references the exact name of the tool to use
No description provided.