-
Notifications
You must be signed in to change notification settings - Fork 676
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 editor tab/indentation settings to OmniSharp #1055
Conversation
This will also address #93. |
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.
Nice change! I just had a question about whether the options could/should be specified as command-line arguments.
@@ -193,6 +202,7 @@ function launchWindows(launchPath: string, cwd: string, args: string[]): LaunchR | |||
} | |||
|
|||
let argsCopy = args.slice(0); // create copy of args | |||
//argsCopy.push('--debug'); |
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.
Why is this added and commented out?
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.
oops my dirty attaching of a debugger 😞
envVars['formattingOptions:useTabs'] = !editorConfig.get('insertSpaces', true); | ||
envVars['formattingOptions:tabSize'] = editorConfig.get('tabSize', 4); | ||
envVars['formattingOptions:indentationSize'] = editorConfig.get('tabSize', 4); | ||
} |
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.
Can't these just be passed as additional arguments? I though OmniSharp's configuration builder would pick these up from the command line.
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.
yes they can - in fact I believe restore packages
is passed like that already if I recall correctly. I can change that for consistency, there is no real reason to use environment variables indeed
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.
That's what I thought -- thanks! Yeah, go ahead and pass them as args for consistency. I have no idea what the logic is for options overriding in the configuration middleware that OmniSharp uses. E.g. do command line args override environment variable or vice versa? I just ran into something similar with MSBuild, so it was on my mind. Hence, the question. 😄
Updated the PR to pass the formatting options using command line instead of env vars. OmniSharp now starts like this, with your editor settings passed in:
If you opt out of the formatting settings sharing, then it will start same as before i.e.:
|
I also created a wiki page https://github.com/OmniSharp/omnisharp-roslyn/wiki/Configuration%20Options - might be useful if folks have questions in the future |
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.
Looks great! Love that it's even smaller.
thanks! |
This should fix #834 and related issues.
This PR ensures that we pass VS Code
editor.*
settings related to tabs/indentation into OmniSharp (through environment variables, which OmniSharp already supports - no changes needed there). This way, when invoking commands such asFormat Document
, we will respect the user set up in his VS Code, rather than imposing OmniSharp defaults (spaces over tabs, 4 spaces indentation).This feature can be opt out from by setting
omnisharp.useEditorFormattingSettings
tofalse
.Additionally, OmniSharp has its own tab vs spaces and indentation rules that can be controlled using
omnisharp.json
. If this file is present in the current working directory, it will still take precedence as it was in the past - no change there.