Skip to content
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

Ignore VS Code default exclusions #38

Closed
TomasHubelbauer opened this issue Apr 25, 2018 · 32 comments
Closed

Ignore VS Code default exclusions #38

TomasHubelbauer opened this issue Apr 25, 2018 · 32 comments
Labels

Comments

@TomasHubelbauer
Copy link
Contributor

Hey, I notice that createFileSystemWatcher does not allow for inserting the list of default VS Code exclusions unlike for example findFiles with its exclude argument you can set to undefined to make it use the default built-in excludes. That means currently this extension reports errors from even node_modules and such and in my case that blows some of my projects up to thousands of errors. You might want to consider trying to somehow extract this exclusion list from VS Code and adjusting your glob (either statically or even better dynamically based on the exclusion list) to ignore these.

@TomasHubelbauer
Copy link
Contributor Author

TomasHubelbauer commented Apr 25, 2018

Actually, my bad, I think createFileSystemWatcher should respect the files.watcherExclude VS Code setting. But it doesn't seem to in your case (I am getting results from node_modules even though it is in my files.watcherExclude and in case of one of my own extensions, findFiles doesn't seem to respect search.excludes). Either that's a bug or my understanding is incomplete, whatever the case, I've asked for clarification in microsoft/vscode#48674.

@DavidAnson
Copy link
Owner

Okay, thanks. FYI, I use this extension with Node apps all the time and never get hits from node_modules. Let’s see how that issue goes.

In the meantime, if there is a public repository you can point me to that reproduces the problem, I would be happy to have a look. Thank you!

@TomasHubelbauer
Copy link
Contributor Author

Maybe this is a problem with my Code then, I'll try to repro without extensions and put up a repro if I can get it.

@TomasHubelbauer
Copy link
Contributor Author

This is probably an issue with my setup somehow. I was not at all able to discern how, but it must be. I will continue trying to figure this out in the linked issue and am closing this one as it doesn't appear to be a problem with this (awesome, BTW) extension, but general glob filtering in my VS Code installation.

@DavidAnson
Copy link
Owner

Ok, thanks, good luck! :)

@borekb
Copy link

borekb commented May 29, 2018

Am I possibly hitting the same issue as @TomasHubelbauer? I get thousands of errors if I open any Markdown file and press Ctrl+T to use the new Markdown workspace symbol search.

The Ctrl+T operation takes relatively long and I get about 9 thousand markdownlint errors in our monorepo, with many hits from node_modules and vendor (PHP thing).

@TomasHubelbauer
Copy link
Contributor Author

TomasHubelbauer commented May 29, 2018

I ended up having to do this hack:

https://github.com/TomasHubelbauer/vscode-markdown-link-suggestions/blob/master/src/extension.ts#L49

The default exclusions just didn't work for me and I wasn't sure it was my setup or a general problem but to have a little more control (and be in line with what I consider to be a more reasonable behavior, I just copy paste the above code to all my extensions.

microsoft/vscode#48674

This is still open as a feature request from me to make the findFiles API better, but I don't think there is intention to change the default behavior to pick up the search.exclude section of default excludes in findFiles. :-/

@borekb
Copy link

borekb commented May 29, 2018

Thanks, Tomas. And do you understand why doing Ctrl+T would kick in markdownlint on all MD files in my workspace?

@TomasHubelbauer
Copy link
Contributor Author

TomasHubelbauer commented May 29, 2018

I think Ctrl+T is dogfed by the extension APIs which don't seem to use the right excludes (I may be totally wrong on this) and as a result you are seeing this behavor.

I think you can "fix it" by adding node_modules to files.exclude (that's what findFiles uses - if I am right and Ctrl+T uses findFiles - by default it should be in search.exclude).

@borekb
Copy link

borekb commented May 29, 2018

Thanks. I still don't understand why Ctrl+T would start markdownlint-ing anything to be honest, seems like two unrelated things to me.

@TomasHubelbauer
Copy link
Contributor Author

That's probably because the symbol search runs the symbol-thingy provider API for all documents in the scope of the workspace (so even the incorrectly not-ignored ones) and as a result of these API invocations/documents being open (even if editors are not shown), hooks in MarkDown lint kick in and run the diagnostics provider so you see stuff in the Problems pane.

But I am only speculating here, I didn't look into it further, the reason I'm sharing this is that I think your experience shares the root of the issue with my original problem.

@borekb
Copy link

borekb commented May 29, 2018

BTW, in my case, files.exclude only contains things I absolutely don't want to see like .git and .DS_Store while search.exclude hides things like node_modules and vendor. In this setup, VSCode symbol navigation actually looks into node_modules and vendor which I prefer as I'm able to navigate to library symbols.

It is less clear to me whether VSCode should treat various MD headings in node_modules as navigable symbols but OK, that's how it behaves. But for markdownlint to also lint those file, that is not desirable I would argue.

So maybe there might be two small issues here?

  1. Linting should not be started by Ctrl+T.
  2. search.exclude should be markdownlint's default filter, not an empty array.

@DavidAnson
Copy link
Owner

DavidAnson commented May 29, 2018

Sorry for the trouble, I’m not sure why you are hitting this but others (like me) are not. The markdownlint extension will lint any file that gets opened by VS Code and claims to be a Markdown file. My current guess is that one of your other extensions or maybe VS Code itself is deciding to open everything in node_modules?

Regardless, markdownlint can now ignore files and directories and that should help here. If you add node_modules/** to the ignore list, that should exclude everything in there. Here is how to do that: https://github.com/DavidAnson/vscode-markdownlint/blob/master/README.md#markdownlintignore

@DavidAnson
Copy link
Owner

Note: I typed that glob on my phone, it may need tweaking to work for real.

@borekb
Copy link

borekb commented May 29, 2018

I'm trying to reproduce on a small example repo and everything behaves correctly. I also have two larger repos where I can reproduce.. As usual :) Thanks @DavidAnson for trying to help!

@borekb
Copy link

borekb commented May 29, 2018

Oh, I was wrong! I thought that most of the invalid hits are in node_modules and vendor folders but it's actually just vendor.

How does the default ignore behavior look like, please? What would make Markdown files in node_modules ignored while those in vendor not?

Here are our workspace settings:

"files.exclude": {
    "**/.git": true,
    "**/.svn": true,
    "**/.hg": true,
    "**/.DS_Store": true
},
"search.exclude": {
    "**/node_modules": true,
    "**/vendor": true,
    "**/dist": true,
    "**/*.js": {"when": "$(basename).ts"},
    "**/*?.js": {"when": "$(basename).tsx"},
    "**/*.js.map": {"when": "$(basename)"}
}

Is .gitignore considered somehow? Our root .gitignore is just:

node_modules
.idea
yarn-error.log

There is just node_modules here. The vendor folder is ignored in a nested .gitignore file, subproject/.gitignore:

/vendor
/node_modules

/dist
/build

Does this possibly matter?

@DavidAnson
Copy link
Owner

Could you maybe create a small repo that shows the problem and I will clone it and have a look? There is still a lot going on above that I’d like to filter out. :)

@DavidAnson
Copy link
Owner

@borekb
Copy link

borekb commented May 29, 2018

@borekb
Copy link

borekb commented May 29, 2018

My plan:

  • v2: adding package.json to see if it can be reproduced with node_modules as well.
  • v3: playing with settings.

Good plan or a waste of time? :)

@borekb
Copy link

borekb commented May 29, 2018

@DavidAnson
Copy link
Owner

I won’t have time to look till this evening. Anything you do between now and then is pure goodness in my book. :)

@DavidAnson
Copy link
Owner

Feature request: Can you please put some files in place so I don’t need to run Composer or Docker?

@borekb
Copy link

borekb commented May 29, 2018

Feature request: Can you please put some files in place so I don’t need to run Composer or Docker?

I could but then it eliminates the option that it could be caused by .gitignore :) But it probably isn't so I'll commit all the files.

@DavidAnson
Copy link
Owner

Just a couple files. Keep it simple, please!

@borekb
Copy link

borekb commented May 29, 2018

Oh, it becomes clearer when removing .gitignore:

image

node_modules is still grayed out. In default settings:

image

@borekb
Copy link

borekb commented May 29, 2018

And this is where I stop understanding it. I have copied both settings to my user settings and added vendor:

image

but I'm still getting hits from vendors, and vendors only:

image

and it is checked by linter while node_modules aren't:

image

Ok, I'm giving up for tonight :) Hopefully you'll be able to make better sense of this than me.

@DavidAnson
Copy link
Owner

Looking at VS Code on my machine, I see the following behavior (always re-open the folder when testing).

Prevents linting *.md from vendor directory, but hides directory in explorer:

"files.exclude": {
    "**/vendor": true
},

Does not exclude *.md from vendor directory:

"search.exclude": {
    "**/vendor": true
}

Also does not exclude:

"files.watcherExclude": {
    "**/vendor/*/**": true
},

Which brings me back to suggesting markdownlint.ignore.

Prevents linting *.md from vendor directory, no effect on explorer, no effect on Ctrl+T:

"markdownlint.ignore": [
    "vendor/**"
]

Code-wise, I confirm that VS Code's handling of Ctrl+T is to invoke vscode.workspace.onDidOpenTextDocument for every Markdown document it scans.
There is no matching call to vscode.workspace.onDidCloseTextDocument. Either or both may be a bug.

I can't yet see a way to tell the Ctrl+T scenario apart from the normal document open scenario from the extension's code.

markdownlint.ignore seems to be the ideal solution for now.

@borekb
Copy link

borekb commented May 30, 2018

Thanks, great research! Do you have any idea why Markdown files in vendor would be processed while those in node_modules not? I have a clean installation of VSCode Insiders with no custom settings so VSCode shouldn't have any special knowledge about node_modules, except for search.exclude and files.watcherExclude supposedly don't influence anything linter-related.

@borekb
Copy link

borekb commented May 30, 2018

@TomasHubelbauer Is your extension impacted similarly? I tried to install it and it reports missing links in various unopened Markdown files.

@DavidAnson
Copy link
Owner

FYI, I think VS Code does have special knowledge and handling. I spent a couple of minutes yesterday and found these things that may be relevant:

@DavidAnson
Copy link
Owner

More FYI, @chrmarti kindly opened an issue for VS Code to look into this: microsoft/vscode#50874

What we’re seeing may not be entirely expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants