Added support for a global omnisharp.json #809

filipw · 2017-03-30T07:44:13Z

Fixes #717

The global omnisharp.json file location is:

On Windows: %APPDATA%\OmniSharp\omnisharp.json
On Mac/Linux: ~/.omnisharp/omnisharp.json

The folder and an empty {} file are created automatically if they are not there.
The live reload logic introduced here #804 also supports this global file, meaning any changes to it are automatically reflected.
The global omnisharp.json file is given a lower precedence than a local one, so global settings can be overridden locally. It is however given a higher precedence than environment variables and command line arguments.

…r settings

DustinCampbell

Looks pretty good. Is APPDATA the right place on Windows?

DustinCampbell · 2017-03-30T13:28:37Z

src/OmniSharp.Host/Services/OmniSharpEnvironment.cs

@@ -38,6 +40,20 @@ public class OmniSharpEnvironment : IOmniSharpEnvironment
            TraceType = traceType;
            TransportType = transportType;
            OtherArgs = otherArgs;
+
+            // On Windows: %APPDATA%\OmniSharp\omnisharp.json


Should this be "%USERPROFILE%.omnisharp\omnisharp.json" to match more closely to the Unix location? Usually, users aren't expected to edit or place files under %APPDATA%.

Wasn't there a huge fight about that when dotnet was still dnx? And they moved things to APPDATA because that was the "windows way"? Some people wanted .dotnet and others wanted it elsewhere?

Personally I like using USERPROFILE myself, but I don't to make things any more confusing than they have to be.

No idea. On my machine, I see the following under USERPROFILE:

.dnx

.node-gyp

.nuget

.templateengine

.vscode

.vscode-insiders

.vscode-oss-dev

I think we're in good company in picking USERPROFILE. 😄

DustinCampbell · 2017-03-30T13:28:47Z

src/OmniSharp.Host/Startup.cs

@@ -20,6 +20,8 @@
 using OmniSharp.Services.FileWatching;
 using OmniSharp.Stdio.Logging;
 using OmniSharp.Stdio.Services;
+using System.IO;
+using OmniSharp.Host.Internal;


Sort usings?

DustinCampbell · 2017-03-30T13:31:39Z

src/OmniSharp.Host/Startup.cs

                .AddEnvironmentVariables();

            if (env.OtherArgs?.Length > 0)
            {
                configBuilder.AddCommandLine(env.OtherArgs);
            }

+            // Use the global omnisharp config if there's any in the shared path
+            configBuilder.CreateAndAddGlobalOptionsFile(env);


It strikes me a little weird that we'd start creating files on the user's machine without them knowing, even if it's an empty file for convenience.

I agree, I was a bit on the fence with this one. The idea was to be able to say: "go here, and edit the omnisharp.json file to have global shared settings", rather than having to say "please create it".

However, you are right, an empty file is a bit silly.

I think we should still create the folder though, it would be somewhat awkward to have to manually create folders that OmniSharp needs.

Other things (not necessarily language servers specificly) do this kind of thing. If the files don't exist create them. Atom or VSCode both on start.

From a UX point of view it would be easier to point the user to a file and say "Edit your global settings here". However since we (omnisharp) aren't really an editor, we could leave that for the editors to implement themselves.

Yup. I gated my review on APPDATA vs. USERPROFILE and not whether we create the file or not. I just commented that it struck me as a little weird. 😄

Shall we keep it as is and change it if we need to?

the local file {working_dir}/omnisharp.json is not implicitly created, and users have to manually add it, so at least in that sense the behavior for the global one is consistent

DustinCampbell · 2017-03-30T13:34:11Z

src/OmniSharp.Host/Internal/OmniSharpConstants.cs

+        internal const string OmnisharpConfigFile = "config.json";
+        internal const string OmnisharpOptionsFile = "omnisharp.json";
+    }
+}


Multiple references to "OmniSharp" seem redundant. Maybe remove it from the member names? Also, the member names have different casing of "OmniSharp". 😄

DustinCampbell · 2017-03-30T13:36:30Z

src/OmniSharp.Host/Internal/ConfigurationBuilderExtensions.cs

+                configBuilder.AddJsonFile(
+                    new PhysicalFileProvider(env.SharedPath),
+                    OmniSharpConstants.OmnisharpOptionsFile,
+                    optional: true,


We make the file optional but always create it anyway?

DustinCampbell · 2017-03-30T13:38:02Z

src/OmniSharp.Host/Services/OmniSharpEnvironment.cs

@@ -1,12 +1,14 @@
 using System;
 using Microsoft.Extensions.Logging;
+using System.IO;


Sort usings

DustinCampbell · 2017-03-30T13:38:44Z

src/OmniSharp.Host/Services/OmniSharpEnvironment.cs


 namespace OmniSharp.Services
 {
    public class OmniSharpEnvironment : IOmniSharpEnvironment
    {
        public string Path { get; }
        public string SolutionFilePath { get; }
+        public string SharedPath { get; }


Should this be SharedDirectoryPath for clarity?

DustinCampbell · 2017-03-30T13:39:52Z

src/OmniSharp.Host/Services/OmniSharpEnvironment.cs

+            var root = Environment.GetEnvironmentVariable("APPDATA") ??
+                Environment.GetEnvironmentVariable("HOME");
+
+            if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("APPDATA")))


Nit: It seems a little funny that we get the environment variable for APPDATA and then get it again immediately.

filipw · 2017-03-30T14:31:28Z

I moved it from APPDATA to USERPROFILE as suggested - the paths are now unified for both OS families ({HOME}|{USERPROFILE}/.omnisharp/omnisharp.json). There is no more automatic file creation, but we still have directory creation there.

Aside from convenience for the user, having the directory there allows us to use the file watcher under the hood of the Options library - as it can watch for a non-existing file, and light up as soon as it's created, which is a nice user experience. This wouldn't be possible if the folder wasn't there.

Also addressed all the other code review feedback.

DustinCampbell

LGTM. Thanks for making those changes!

david-driscoll

Looks good, I have a question on ordering of local config vs command line args vs environment args but that isn't directly related to this change.

david-driscoll · 2017-03-30T14:56:22Z

src/OmniSharp.Host/Services/OmniSharpEnvironment.cs

@@ -38,6 +40,20 @@ public class OmniSharpEnvironment : IOmniSharpEnvironment
            TraceType = traceType;
            TransportType = transportType;
            OtherArgs = otherArgs;
+
+            // On Windows: %APPDATA%\OmniSharp\omnisharp.json


Wasn't there a huge fight about that when dotnet was still dnx? And they moved things to APPDATA because that was the "windows way"? Some people wanted .dotnet and others wanted it elsewhere?

Personally I like using USERPROFILE myself, but I don't to make things any more confusing than they have to be.

david-driscoll · 2017-03-30T15:00:52Z

src/OmniSharp.Host/Startup.cs

                .AddEnvironmentVariables();

            if (env.OtherArgs?.Length > 0)
            {
                configBuilder.AddCommandLine(env.OtherArgs);
            }

+            // Use the global omnisharp config if there's any in the shared path
+            configBuilder.CreateAndAddGlobalOptionsFile(env);


Other things (not necessarily language servers specificly) do this kind of thing. If the files don't exist create them. Atom or VSCode both on start.

From a UX point of view it would be easier to point the user to a file and say "Edit your global settings here". However since we (omnisharp) aren't really an editor, we could leave that for the editors to implement themselves.

DustinCampbell · 2017-03-31T13:02:37Z

I think we're in good shape here. @david-driscoll, holler if you disagree.

DustinCampbell · 2017-03-31T13:22:34Z

@filipw: I think this change warrants an update to https://github.com/OmniSharp/omnisharp-roslyn/wiki/Configuration-Options.

filipw · 2017-03-31T14:06:40Z

indeed 📚

filipw added 4 commits March 29, 2017 14:10

added support for a global omnisharp.json

7b9c743

changed SharedPAth to be initialized in the constructor like the othe…

01c4a09

…r settings

create shared directory if not exists

f9fdecc

ensure the omnisharp.json file is created

9129db7

filipw requested review from DustinCampbell and david-driscoll March 30, 2017 07:44

removed unused namespace

0b8df27

DustinCampbell requested changes Mar 30, 2017

View reviewed changes

addressed code review feedback

875ba93

DustinCampbell approved these changes Mar 30, 2017

View reviewed changes

david-driscoll approved these changes Mar 30, 2017

View reviewed changes

Merge branch 'dev' into features/global-omnisharp.json

648366e

DustinCampbell merged commit 3c3a8e5 into OmniSharp:dev Mar 31, 2017

filipw deleted the features/global-omnisharp.json branch March 31, 2017 14:06

MicahZoltu mentioned this pull request Aug 28, 2017

Be a good citizen on Windows (don't dump config into %USERPROFILE%). #953

Open

ssoher mentioned this pull request Feb 11, 2022

[Question] How to change format settings for C# code? dotnet/vscode-csharp#5036

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for a global omnisharp.json #809

Added support for a global omnisharp.json #809

filipw commented Mar 30, 2017 •

edited

Loading

DustinCampbell left a comment

DustinCampbell Mar 30, 2017

david-driscoll Mar 30, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

filipw Mar 30, 2017 •

edited

Loading

david-driscoll Mar 30, 2017

DustinCampbell Mar 30, 2017

filipw Mar 31, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

DustinCampbell Mar 30, 2017

filipw commented Mar 30, 2017

DustinCampbell left a comment

david-driscoll left a comment

david-driscoll Mar 30, 2017

david-driscoll Mar 30, 2017

DustinCampbell commented Mar 31, 2017

DustinCampbell commented Mar 31, 2017

filipw commented Mar 31, 2017

Added support for a global omnisharp.json #809

Added support for a global omnisharp.json #809

Conversation

filipw commented Mar 30, 2017 • edited Loading

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Mar 30, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Mar 31, 2017

DustinCampbell commented Mar 31, 2017

filipw commented Mar 31, 2017

filipw commented Mar 30, 2017 •

edited

Loading

filipw Mar 30, 2017 •

edited

Loading