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

A few small option description tweaks #2239

Closed
wants to merge 2 commits into from

Conversation

DustinCampbell
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented May 1, 2018

Codecov Report

Merging #2239 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2239      +/-   ##
==========================================
+ Coverage    59.7%   59.75%   +0.05%     
==========================================
  Files          77       77              
  Lines        3762     3762              
  Branches      543      545       +2     
==========================================
+ Hits         2246     2248       +2     
+ Misses       1342     1341       -1     
+ Partials      174      173       -1
Flag Coverage Δ
#integration 51.05% <ø> (+0.05%) ⬆️
#unit 82.61% <ø> (ø) ⬆️
Impacted Files Coverage Δ
src/omnisharp/delayTracker.ts 73.68% <0%> (+5.26%) ⬆️

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 94d07b6...7e5c2df. Read the comment docs.

@@ -521,7 +521,7 @@
"omnisharp.useMono": {
"type": "boolean",
"default": false,
"description": "Launch OmniSharp with Mono."
"description": "Launch OmniSharp with Mono. Note that this option is only valid when 'omnisharp.path' is set to an absolute path."

Choose a reason for hiding this comment

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

Is this not true if path is latest or a specific version number?

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand from the code we are trying to use the mono irrespective of whether the omnisharp.path is set and if set what is the value. The launcher as of now just cares that if the useMono is set then it will try to launch using mono: https://github.com/OmniSharp/omnisharp-vscode/blob/master/src/omnisharp/launcher.ts#L245

Copy link

Choose a reason for hiding this comment

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

Maybe the docs should explicitly visualize the table we're talking about :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@akshita31: Yes, there is essentially a bug that I think we should tweak here. Your PR to make the default OmniSharp use the same infrastructure as everything else actually makes it easier to fix that. I'll go ahead and do it.

@DustinCampbell
Copy link
Member Author

Closed in favor of #2244

@DustinCampbell DustinCampbell deleted the option-cleanup branch May 1, 2018 21:34
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.

4 participants