-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix ignore minimatch to include folders starting with dot #8074
Conversation
@amiramw Could you check please why build is failing? |
@akosyakov can you help me understand why is it failing? It's not clear to me from the logs. is it stable? I see that it only failed on linux. maybe should try to retrigger? |
for example during npm install there is a temp folder called .staging and we don't want to listen to those files Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
@akosyakov after rebase it seems to pass |
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.
@amiramw do you perhaps have the code of the extension hosted anywhere (ex: github repository), I wanted to see what it does or expects. I'm not sure what the pull-request is trying to fix, for example when a package.json
file under .bin
is opened it is being watched:
root INFO [nsfw-watcher: 15227] Started watching: /home/evinfug/workspaces/theia/node_modules/.bin/package.json
root INFO [nsfw-watcher: 15227] Stopped watching: /home/evinfug/workspaces/theia/node_modules/.bin/package.json
What problem is the pull-request solving, it was not clear to me by the description.
@vince-fugnitto the git repo for this test extension is https://github.com/amiramw/vscode-file-watch-package-json It basically listen to package.json file changes and print those events to output: const watcher = vscode.workspace.createFileSystemWatcher("**/package.json");
const outputChannel = vscode.window.createOutputChannel('Package.json Events');
watcher.onDidCreate(uri => outputChannel.appendLine('Created ' + uri.toString()));
watcher.onDidChange(uri => outputChannel.appendLine('Changed ' + uri.toString())); nsfw listens to every file change however theia/packages/filesystem/src/node/nsfw-watcher/nsfw-filesystem-watcher.ts Lines 234 to 237 in 7f729f2
The ignored glob pattern is in settings.json and the default value is: "files.watcherExclude": {
"**/.git/objects/**": true,
"**/.git/subtree-cache/**": true,
"**/node_modules/**": true
} My fix is to configure the ignore glob pattern minimatch so that it will also identify folders starting with dot in the path. Otherwise events are fired also for paths inside node_modules which is wrong and incompatible with vs code. The vscode extension I provided test that. |
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.
@amiramw thank you for taking the time to describe the issue further and the use of dot
.
Logically the change makes sense and I verified that the logs are not outputted to the output channel with your updates.
for example during npm install there is a temp folder called .staging and we don't want to listen to those files.
Signed-off-by: Amiram Wingarten amiram.wingarten@sap.com
What it does
Add minimatch dot option to make sure also folders starting with dot are excluded from watched. See also https://github.com/isaacs/minimatch#dot
The new behavior complies also with VS Code watch ignore behavior.
How to test
Before this PR there was an output message to output channel "Package.json Events" which is wrong. After this PR there is no such ourput.
Review checklist
Reminder for reviewers