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

Use workspace.findFiles instead of glob #3681

Merged
merged 4 commits into from
Mar 19, 2020
Merged

Conversation

JoeRobich
Copy link
Member

@JoeRobich JoeRobich commented Mar 18, 2020

isBlazorWebAssemblyProject used glob to search for launchSettings.json files. It was being invoked 3 different times for each project within the workspace by requestWorkspaceInformation.

Based on this advice:

	 * The workspace offers support for [listening](#workspace.createFileSystemWatcher) to fs
	 * events and for [finding](#workspace.findFiles) files. Both perform well and run _outside_
	 * the editor-process so that they should be always used instead of nodejs-equivalents.

Updated isBlazorWebAssemblyProject to use vscode.workspace.findFiles and also use the excluded paths defined in files.exclude

Potential fix for #3673

@JoeRobich JoeRobich requested a review from NTaylorMullen March 18, 2020 22:09
@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #3681 into master will increase coverage by 0.11%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3681      +/-   ##
==========================================
+ Coverage    89.8%   89.91%   +0.11%     
==========================================
  Files          59       59              
  Lines        1589     1597       +8     
  Branches       89       89              
==========================================
+ Hits         1427     1436       +9     
+ Misses        151      150       -1     
  Partials       11       11
Flag Coverage Δ
#integration 100% <ø> (?)
#unit 89.91% <92.3%> (+0.11%) ⬆️
Impacted Files Coverage Δ
src/omnisharp/options.ts 94.73% <92.3%> (+2.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02ba0dd...bf46264. Read the comment docs.

return excludePaths;
}

let excludeFilesOption = workspaceConfig.get<{ [i: string]: boolean }>('files.exclude');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity does VSCode pre-fill node_modules in this list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately it wasn't set for the dotnet new razor project I created to test with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense for us to auto-include node_modules and whatever Unity's package folder is if it doesn't already exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should also include search.exclude.. hmm..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good news is that Unity seems to create a .vscode/settings.json which has an extensive files.exclude defined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to include search.exclude paths which by default include node_modules. Unity's settings.json includes the Library/PackageCache in its files.exclude so I think we are covered from these scenarios.

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

Successfully merging this pull request may close these issues.

2 participants