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

Pass a semicolon-delimited list of files excluded from VS Code. #2171

Merged
merged 8 commits into from
May 29, 2019

Conversation

rchande
Copy link

@rchande rchande commented Apr 10, 2018

Fixes #1990

@codecov
Copy link

codecov bot commented Apr 10, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@8b02101). Click here to learn what that means.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2171   +/-   ##
=========================================
  Coverage          ?   88.72%           
=========================================
  Files             ?       59           
  Lines             ?     1587           
  Branches          ?       89           
=========================================
  Hits              ?     1408           
  Misses            ?      168           
  Partials          ?       11
Flag Coverage Δ
#integration 100% <ø> (?)
#unit 88.72% <60%> (?)
Impacted Files Coverage Δ
src/omnisharp/options.ts 89.23% <60%> (ø)

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 8b02101...9117912. Read the comment docs.

@TheRealPiotrP
Copy link

Can we make the list an array instead of a string? This will be easier for users to edit, to diff..

@TheRealPiotrP
Copy link

Also, a test would be useful :)

@TheRealPiotrP
Copy link

And are the files consisting of relative paths? Relative to what? What happens if I switch launch targets?

@rchande
Copy link
Author

rchande commented Apr 11, 2018

@TheRealPiotrP

Can we make the list an array instead of a string? This will be easier for users to edit, to diff..

Users define this data by editing artifacts in their repo or in vs code settings. The string generated here will be passed to O# as a command line arg. Arguably, it could be json, but that's not very user-friendly for anyone invoking o# outside of the C# extension.

And are the files consisting of relative paths? Relative to what? What happens if I switch launch targets?

They're paths relative to the workspace root. Maybe there's some normalizing we need to do if the working directory for o# isn't the workspace root.

Also, a test would be useful :)

Ok fine :)

Copy link
Contributor

@filipw filipw left a comment

Choose a reason for hiding this comment

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

Since OmniSharp now has built in support for globbing, and that is exposed over the standard OmniSharp configuration system, you should use that instead, instead of creating custom arguments. Then things will just work automatically, without any other changes required.

OmniSharp allows overriding of its configuration if you pass the args like this:

omnisharp {any other args} fileOptions:excludeSearchPatterns:0=**/foo/**/* fileOptions:excludeSearchPatterns:1=**/bar/**/*

Note that this exact approach is already used for passing formatting options when starting OmniSharp from the extension.

@filipw
Copy link
Contributor

filipw commented May 4, 2018

actually, based on my own comment here OmniSharp/omnisharp-roslyn#1161 (comment) you should pass those settings as fileOptions: systemExcludeSearchPatterns not fileOptions:excludeSearchPatterns as that would allow the users to still incorporate their own exclusions via env variables or omnisharp.json

@firelizzard18
Copy link

Is this still being worked?

@akshita31
Copy link
Contributor

cc @rchande

@kvalev
Copy link

kvalev commented Dec 11, 2018

@rchande Do you have the time to finalize this PR? If not I can give it a go.

@rchande
Copy link
Author

rchande commented Dec 11, 2018

@kvalev Thanks for the ping.
@filipw I've implemented your suggestion to use the normal omnisharp configuration mechanism. Any other feedback?

@Blackbaud-MitchellThomas

I am interested in this feature. We want to turn on the system diagnostics but want to exclude certain generated code.

Copy link
Contributor

@filipw filipw left a comment

Choose a reason for hiding this comment

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

LGTM! sorry for the delay, I somehow missed your message 😀
just needs the conflict resolution and it should be good to go

@WeAthFoLD
Copy link

Is this still being worked on? It would be a very useful feature.

@rchande
Copy link
Author

rchande commented May 29, 2019

Better late than never, this is ready to merge.

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.

Extension complains about project.json files in node_modules
8 participants