-
Notifications
You must be signed in to change notification settings - Fork 418
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
Added support for a global omnisharp.json #809
Changes from 6 commits
7b9c743
01c4a09
f9fdecc
9129db7
0b8df27
875ba93
648366e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,4 +15,4 @@ internal static T GetValueOrDefault<T>(this CommandOption opt, T defaultValue) | |
return defaultValue; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
using Microsoft.Extensions.Configuration; | ||
using Microsoft.Extensions.FileProviders; | ||
using OmniSharp.Services; | ||
using System; | ||
using System.IO; | ||
|
||
namespace OmniSharp.Host.Internal | ||
{ | ||
internal static class ConfigurationBuilderExtensions | ||
{ | ||
internal static void CreateAndAddGlobalOptionsFile(this IConfigurationBuilder configBuilder, IOmniSharpEnvironment env) | ||
{ | ||
if (env?.SharedDirectoryPath == null) return; | ||
|
||
try | ||
{ | ||
if (!Directory.Exists(env.SharedDirectoryPath)) | ||
{ | ||
Directory.CreateDirectory(env.SharedDirectoryPath); | ||
} | ||
|
||
var omnisharpGlobalFilePath = Path.Combine(env.SharedDirectoryPath, Constants.OptionsFile); | ||
configBuilder.AddJsonFile( | ||
new PhysicalFileProvider(env.SharedDirectoryPath), | ||
Constants.OptionsFile, | ||
optional: true, | ||
reloadOnChange: true); | ||
} | ||
catch (Exception e) | ||
{ | ||
// at this point we have no ILogger yet | ||
Console.Error.WriteLine($"There was an error when trying to create a global '{Constants.OptionsFile}' file in '{env.SharedDirectoryPath}'. {e.ToString()}"); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
namespace OmniSharp.Host.Internal | ||
{ | ||
internal static class Constants | ||
{ | ||
internal const string ConfigFile = "config.json"; | ||
internal const string OptionsFile = "omnisharp.json"; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Multiple references to "OmniSharp" seem redundant. Maybe remove it from the member names? Also, the member names have different casing of "OmniSharp". 😄 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
using Microsoft.Extensions.FileProviders; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.Extensions.Options; | ||
using OmniSharp.Host.Internal; | ||
using OmniSharp.Mef; | ||
using OmniSharp.Middleware; | ||
using OmniSharp.Options; | ||
|
@@ -36,18 +37,21 @@ public Startup(IOmniSharpEnvironment env) | |
|
||
var configBuilder = new ConfigurationBuilder() | ||
.SetBasePath(AppContext.BaseDirectory) | ||
.AddJsonFile("config.json", optional: true) | ||
.AddJsonFile(Constants.ConfigFile, optional: true) | ||
.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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the local file |
||
|
||
// Use the local omnisharp config if there's any in the root path | ||
configBuilder.AddJsonFile( | ||
new PhysicalFileProvider(env.Path), | ||
"omnisharp.json", | ||
Constants.OptionsFile, | ||
optional: true, | ||
reloadOnChange: true); | ||
|
||
|
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.
We make the file optional but always create it anyway?