-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Configuration -> RC2 #1231
Configuration -> RC2 #1231
Conversation
@divega @shirhatti Please review |
@danroth27 do you want me to try to incorporate all of the outstanding config issues into this PR, or can we accept this PR and then track the other issues with separate, additional PRs (my preference)? |
We don't need to fix everything with the config article in this PR - this PR is for updating the article to be consistent for RC2. |
Cool, then this is, still, ready for review and acceptance... |
I am going to take a look. @HaoK can you take a look too? |
Is there an easy way to review this as the documentation will look? |
@HaoK You can view the RC1 version here: https://docs.asp.net/en/latest/fundamentals/configuration.html I can email you an HTML version of this PR if you let me know your email address. |
} | ||
}, | ||
"runtimes": { |
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.
For .NET Core apps you shouldn't need to specify runtimes explicitly.
A couple of minor comments, but otherwise |
@danroth27 the samples don't work without the runtimes section. "Can not find runtime target for framework '.NETCoreApp, Version=v1.0' compatible with one of the target runtimes... in response to dotnet build. |
"Microsoft.Extensions.Configuration.EnvironmentVariables": "1.0.0-rc1-final", | ||
"Microsoft.Extensions.Configuration.Json": "1.0.0-rc1-final" | ||
}, | ||
"NETStandard.Library": "1.5.0-rc2-24027", |
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.
Replace with Microsoft.NETCore.App: { "type" = "platform", "version" = "blah" }
Looks like we should actually change provider -> source pretty much everywhere in this doc. You now add config sources to your config builder, not config providers. Providers only come into play when you implement a custom config source. You implement a custom provider as part of implementing a custom config source. |
3c99141
to
74140e2
Compare
<Import Project="$(VSToolsPath)\DNX\Microsoft.DNX.Props" Condition="'$(VSToolsPath)' != ''" /> | ||
<PropertyGroup Label="Globals"> | ||
<ProjectGuid>8ee63173-bff1-4e78-ab94-49a5be0eb2d8</ProjectGuid> | ||
<RootNamespace>ConfigConsole</RootNamespace> | ||
<BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)'=='' ">..\..\artifacts\obj\$(MSBuildProjectName)</BaseIntermediateOutputPath> | ||
<OutputPath Condition="'$(OutputPath)'=='' ">..\..\artifacts\bin\$(MSBuildProjectName)\</OutputPath> | ||
<OutputPath Condition="'$(OutputPath)'=='' ">.\bin\</OutputPath> |
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.
Nit: Not sure why did VS do this (it happened to me when working on #1301 too), but it means that obj
and bin
are now in different places.
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.
Does this matter? What would I update it to, .\obj\ ?
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.
Does this matter?
Not sure.
What would I update it to, .\obj\ ?
In my PR, I reverted the change, but it's possible VS will want to change it again.
So I guess changing BaseIntermediateOutputPath
to .\obj
(which seems to be the default for new projects) makes sense.
9f868d0
to
65233a8
Compare
Got previous squirrel so merged this. @svick please use separate issue for any other updates required/suggested. Thanks! |
Fixes #647