-
Notifications
You must be signed in to change notification settings - Fork 131
Assistant: remove directory structure tool #10870
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
Conversation
...in favor of a `directoriesOnly` argument to `getProjectTree`.
|
E2E Tests 🚀 |
| maxFiles?: number; | ||
| directoriesOnly?: boolean; |
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.
would we still want something like maxDepth when directoriesOnly is used? Or maybe we'd want to switch maxFiles --> maxItems and use it for both file mode and directory mode?
| throw new Error(`The 'include' parameter is required. Specify glob patterns to target specific files (e.g., ["src/**/*.py"], ["*.ts", "tests/**"]).`); | ||
| } | ||
|
|
||
| const filePatterns = include; |
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.
If we do maxFiles --> maxItems, should we rename these variables as well? E.g. filePatterns --> globPatterns; filesLimit --> itemLimit, etc.
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.
Yes, thank you! c80f2f7
| if (!skipExcludes && totalFiles < sparseThreshold) { | ||
| log.debug(`[${PositronAssistantToolName.ProjectTree}] Default exclusions were applied and results were very sparse. Searching files again to determine how many files were excluded...`); | ||
| const allMatchesUris = await vscode.workspace.findFiles2( | ||
| filePatterns, | ||
| { | ||
| exclude: excludePatterns.length > 0 ? excludePatterns : undefined, | ||
| useIgnoreFiles: { | ||
| local: false, | ||
| parent: false, | ||
| global: false, | ||
| }, | ||
| useExcludeSettings: vscode.ExcludeSettingOptions.None, | ||
| }, | ||
| token | ||
| ); | ||
| const totalFilesBeforeExclusion = allMatchesUris.length; | ||
| excludedCount = totalFilesBeforeExclusion - totalFiles; | ||
| } |
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.
When directoriesOnly is specified, should we still use this logic? As it will use findFiles instead of collecting directories
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 ought not to have missed that. Thank you! 24f378b
Co-authored-by: sharon <[email protected]> Signed-off-by: Simon P. Couch <[email protected]>
I noticed that the second, unrestricted search in case of sparse result could take a while if there were many results, so I opted to stop after _any_ additional results were found in the second-search case and then just note that _something_ was found. This cuts down on the latency quite a bit, and the model seems to still take the hint that it should keep looking.
sharon-wang
left a comment
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! 📁 📁 📁
| } | ||
|
|
||
| if (matchesInclude(relativePath, includePatterns)) { | ||
| directories.push(relativePath + '/'); |
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 wondered if the / might be an issue for Windows, but it seems that asRelativePath (which calls getRelativePath) ultimately uses posix forward slashes /, so we should be good here!
positron/src/vs/workbench/api/common/extHostWorkspace.ts
Lines 387 to 421 in 9873233
| getRelativePath(pathOrUri: string | vscode.Uri, includeWorkspace?: boolean): string { | |
| let resource: URI | undefined; | |
| let path: string = ''; | |
| if (typeof pathOrUri === 'string') { | |
| resource = URI.file(pathOrUri); | |
| path = pathOrUri; | |
| } else if (typeof pathOrUri !== 'undefined') { | |
| resource = pathOrUri; | |
| path = pathOrUri.fsPath; | |
| } | |
| if (!resource) { | |
| return path; | |
| } | |
| const folder = this.getWorkspaceFolder( | |
| resource, | |
| true | |
| ); | |
| if (!folder) { | |
| return path; | |
| } | |
| if (typeof includeWorkspace === 'undefined' && this._actualWorkspace) { | |
| includeWorkspace = this._actualWorkspace.folders.length > 1; | |
| } | |
| let result = relativePath(folder.uri, resource); | |
| if (includeWorkspace && folder.name) { | |
| result = `${folder.name}/${result}`; | |
| } | |
| return result!; | |
| } |
positron/src/vs/base/common/resources.ts
Lines 235 to 258 in 9873233
| relativePath(from: URI, to: URI): string | undefined { | |
| if (from.scheme !== to.scheme || !isEqualAuthority(from.authority, to.authority)) { | |
| return undefined; | |
| } | |
| if (from.scheme === Schemas.file) { | |
| const relativePath = paths.relative(originalFSPath(from), originalFSPath(to)); | |
| return isWindows ? extpath.toSlashes(relativePath) : relativePath; | |
| } | |
| let fromPath = from.path || '/'; | |
| const toPath = to.path || '/'; | |
| if (this._ignorePathCasing(from)) { | |
| // make casing of fromPath match toPath | |
| let i = 0; | |
| for (const len = Math.min(fromPath.length, toPath.length); i < len; i++) { | |
| if (fromPath.charCodeAt(i) !== toPath.charCodeAt(i)) { | |
| if (fromPath.charAt(i).toLowerCase() !== toPath.charAt(i).toLowerCase()) { | |
| break; | |
| } | |
| } | |
| } | |
| fromPath = toPath.substr(0, i) + fromPath.substr(i); | |
| } | |
| return paths.posix.relative(fromPath, toPath); | |
| } |

Addresses #10721, reworking the changes from #10517. Removes
getDirectoryStructurein favor of adirectoriesOnlyargument togetProjectTreeto save on input tokens and reduce the total number of tools.The same examples from the PR that originally introduced the tool, this time using
getProjectTreewithdirectoriesOnly. Efficiently exploring the root:Navigating a folder that's excluded by default by noticing that results were excluded and calling the tool again:
Seems like models can get just as much mileage out of this interface as with the dedicated tool.
Release Notes
New Features
getDirectoryStructuretool #10721)Bug Fixes
QA Notes