-
Notifications
You must be signed in to change notification settings - Fork 216
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
Support nested ignore files #1634
Conversation
@abeatrix these tests are all green now (although there's a TODO about which URI the Ignore tests should be using). I had a quick test and creating an ignore file did exclude files from the automatic context. I couldn't get the file selection at the bottom of chat working though, it seems to require the app (which I understand isn't available on Windows?) |
@DanTup thank you so much for fixing the tests, and getting nested ignore file to work in less than a day 🤯 Regarding the file selection at the bottom below the chat, it doesn't required App to work iirc. It gets updated to whatever file you're currently on but it's not being sent to the llm. I can look into it more on Monday unless you are referring to something 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.
LGTM! I love how well documented everything is in your code, something I really need to learn from you! I can look into the file selection thing more on Monday plus I know you're about to start on the plg work soon, so feel free to merge this whenever, and we can put this behind a feature flag for internal users and other clients to dogfood, what do you think?
const candidates = Array.from(this.workspaceIgnores.keys()).filter(workspaceRoot => | ||
filePath.toLowerCase().startsWith(workspaceRoot.toLowerCase()) | ||
) | ||
// If this file was inside multiple workspace roots, take the shortest one since it will 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.
🧠
I tested the automatically file selection - that seemed to work (the "Read 3 files" but excluded my current file after I added to the ignore list). However there was another button underneath that seemed like it was to let me manually select the files, but I couldn't get it to work (it said it wanted the app). I might have misunderstood that functionality though. I'll merge this into your branch and let you add the feature flag before merging to master then. If I run out/get blocked on PLG stuff I can finish if needed. Let me know if you have an issues/questions with the bits I did. Some tests I hadn't written yet but thought would be valuable (probably they'd need to be integration tests):
|
Test plan