Fix a bug not to read rule files properly, under nested .roo/rules directories
#2405
Conversation
🦋 Changeset detectedLatest commit: e9c7bed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
the
size:M
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| .readdir(dirPath, { withFileTypes: true, recursive: true }) | ||
| .then((files) => files.filter((file) => file.isFile())) | ||
| .then((files) => files.map((file) => path.resolve(dirPath, file.name))) | ||
| .then((files) => files.map((file) => path.resolve(file.parentPath, file.name))) |
There was a problem hiding this comment.
Using file.parentPath assumes that the Dirent includes a parentPath property, which is non-standard. Consider defaulting to the original dirPath if parentPath is undefined.
| .then((files) => files.map((file) => path.resolve(file.parentPath, file.name))) | |
| .then((files) => files.map((file) => path.resolve(file.parentPath || dirPath, file.name))) |
There was a problem hiding this comment.
Indeed Dirent#parentPath is marked as Stability:1.
https://nodejs.org/docs/latest-v20.x/api/fs.html#direntparentpath
Stability: 1 - Experimental. The feature is not subject to semantic versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.
Considering Dirent#path was the original function name but deprecated now, the new function name parentPath seems to be still the first option to use.
It might be arguable that we may want to use just parentPath, or prepare some fail-overs.
- stay
file.parentPath file.parentPath || dirPathfile.parentPath || file.path- or calculate fullpath in a recursive concatenation manner (
subdir/+subdir2/+file1.txt)
Which pattern do you prefer?
There was a problem hiding this comment.
Oh nice, I'm not sure I have any strong opinions. What do you think is best? cc: @upanume who wrote this originally.
There was a problem hiding this comment.
I implemented this! Thank you for the bug fix 😭
IMO: While there are certainly points of discussion, I think either "stay file.parentPath" or "file.parentPath || dirPath" would be good. If the behavior changes due to Node.js modifications, the tests would fail, so we would be able to notice, and I don't think we need to make things unnecessarily complicated.
mrubens
left a comment
mrubens
left a comment
There was a problem hiding this comment.
I think this is good as is. Thank you!
|
Wow, thank you for your prompt review&merge! 🚀 |
|
Thank you for the contribution! |
* Create Tailwind DangerButton * changeset * remove unused import * Remove old DangerButton component * rename DangerButtonTW to DangerButton
3 participants
Context
Fix a bug not to resolve nested
.roo/rulesdirectories properly, such as.roo/rules/subdir/file1.txtor.roo/rules/subdir/subdir2/filerule files.It seems to be intended to support nested directory structures, as
recursiveoption is marked astruein the implementation.Sub directories are quite helpful to organize multiple custom instructions files. It is nicer to support.
Implementation
Use
Dirent#parentPathinstead ofdirPathto resolve full path of each rule file.This is because
dirPathwould be the path of.roo/rules, not sub directory path. So current path misses all subdirectories.should be
Screenshots
How to Test
Preview System Promptand check if.roo/rules/subdir/missed.txtis included.Get in Touch
I am in the Roo Code Discord server as
@taisukeoe. Please feel free to ask if any!