add fileUri #1672
add fileUri #1672
Conversation
| // Skip embeddings results as they should be filtered when it was indexed | ||
| if (message.file.source === 'embeddings') { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Will embeddings have fileUri at all? If not (if it's made a rule that fileUri is only ever set if it's absolute) then the if condition above might handle this?
| @@ -9,6 +11,9 @@ export type ContextFileSource = 'embeddings' | |||
|
|
|||
| export interface ContextFile { | |||
| fileName: string | |||
|
|
|||
| fileUri?: URI | |||
There was a problem hiding this comment.
I wonder if we should just call this uri? In future, maybe documents aren't all from the file system with a file path (VS Code supports various schemes and workspaces without files), so it might be less to change later even if today they're all file:// URIs?
| // return early if the file is on ignore list | ||
| if (isCodyIgnoredFile(match.fileName)) { | ||
| // TODO: makes sure fileUri is included for all context snippets | ||
| if (match.fileUri?.fsPath && isCodyIgnoredFile(match.fileUri?.fsPath)) { |
There was a problem hiding this comment.
I also wonder if we should make isCodyIgnoredFile take a URI? 🤔
One one hand, it's harder to pass a relative path (it's probably more obvious that a file URI should be absolute than a path), but on the other hand you're less likely to pass a non-file URI this way (and the implementation would have to throw for non-file URIs for now if we accepted URIs).
There was a problem hiding this comment.
Yea I like this idea a lot. It's much clear than passing in a string that could be anything and much less confusing for the caller and the users! Will update it first thing tomorrow!
| @@ -37,7 +38,7 @@ export class AgentEditor implements Editor { | |||
| if (this.agent.workspace.activeDocumentFilePath === null) { | |||
| return undefined | |||
| } | |||
| if (isCodyIgnoredFile(this.agent.workspace.activeDocumentFilePath)) { | |||
| if (isCodyIgnoredFile(URI.parse(this.agent.workspace.activeDocumentFilePath))) { | |||
There was a problem hiding this comment.
I think these will need to be URI.file() to work on Windows.
I'll merge this into my branch and fix up there and see what the tests are like (it might also be easier than having so many layers or PRs 😄)
Test plan