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

Prefix OmniSharp-specific configuration environment variables with "OMNISHARP_" #872

Merged
merged 2 commits into from
May 25, 2017

Conversation

filipw
Copy link
Member

@filipw filipw commented May 25, 2017

This is a breaking change, but of - I believe - small impact because it's very much a power feature to feed configuration into OmniSharp via environment variables (pretty much all current use cases would be via omnisharp.json or command line arguments).

However, we still allow to override any of the configuration via environment variables.
Because we didn't prefix OmniSharp specific env variables, it can lead to unexpected results like dotnet/vscode-csharp#1512.

This PR requires that OmniSharp uses for configuration only environment variables following this format: OMNISHARP_{name}.
Fixes dotnet/vscode-csharp#1512.

Copy link
Contributor

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

👍

Yes, this is a breaking change, but I think it's a good one. Honestly, I doubt many are doing this today and it fixes weird problems when people have environment variables set for things like "msbuild". Once this goes in, we'll want to update the Configuration Options wiki page as well.

Copy link
Member

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

While it's a breaking change... it's a useful one. 👍

@david-driscoll
Copy link
Member

update the branch for ci.

@DustinCampbell DustinCampbell merged commit 89fa23c into OmniSharp:dev May 25, 2017
@filipw
Copy link
Member Author

filipw commented May 25, 2017

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.

3 participants