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

Update Configuration doc to use double underscores instead of colons for hierarchical config keys #4968

Closed
danroth27 opened this issue Dec 11, 2017 · 18 comments
Assignees
Milestone

Comments

@danroth27
Copy link
Member

Currently the config doc tells you to use colons for hierarchical config keys, which don't work on all platforms. Let's switch to using double scores and then update the note about double underscores to say that colons can also be used, but may not work on all platforms.

@guardrex
Copy link
Collaborator

Related to (or dup of) #4927.

@Rick-Anderson Rick-Anderson added this to the Sprint 130 (ends 1/26/2018) milestone Dec 12, 2017
@isaacrlevin
Copy link
Contributor

@danroth27 do you want the samples in this doc updated as well? Or just lines 46 and 53?

@guardrex
Copy link
Collaborator

Note that the topic is getting an (unrelated) update. It could get messy before that merges 👉 #5169

@isaacrlevin
Copy link
Contributor

@guardrex is there an interest in hitting this low hanging fruit in that PR?

@guardrex
Copy link
Collaborator

I don't think so ... it's approved. It should merge any second. I just meant you could work with a clean, updated topic as soon as that goes in. Merge conflicts are something I really try to avoid. 😨 lol

@isaacrlevin
Copy link
Contributor

ohhhhh maybe I should actually click the link ;)

@guardrex
Copy link
Collaborator

@isaac2004 #5169 is merged now, so if they give you the green light, ur good.

@guardrex
Copy link
Collaborator

Note that on #5241 @scottaddie remarks for aspnetcore/fundamentals/configuration/index.md where it states ...

If a colon (:) can't be used in environment variables on a system, replace the colon (:) with a double-underscore (__).

I recommend adding an example of where the double underscore may be necessary. Maybe we have this documented somewhere already, in which case you could just link to that info.

@scottaddie
Copy link
Member

scottaddie commented Jan 24, 2018

@danroth27 @guardrex I'm hoping we can provide a bit more guidance in this statement. For example, which specific platforms are known to need the double underscore? It's going to be a question in the Feedback section of the doc if we don't specify.

@rachelappel
Copy link
Contributor

@guardrex I was looking around and don't see it documented anywhere. Agree with @scottaddie that we could use more guidance here and which platforms and all that or else the question tidal wave will hit us.

@guardrex
Copy link
Collaborator

@danroth27's OP ☝️ suggests that making every topic go with the double-underscore approach should (in theory) just about eliminate concern over platform behavior. However, I agree that being able to say which platforms have trouble with colons would be helpful.

@rachelappel
Copy link
Contributor

@guardrex I only see him mentioning the config doc. Did I miss something?
Totally agree that consistency is going to be a better thing, so if there are multiple docs that could use the mod we should look at those too.

@guardrex
Copy link
Collaborator

Nah ... just wondering if they'll be found in a few other topics/samples floating around the repo. Hopefully not, and then this won't be a big deal.

@isaacrlevin
Copy link
Contributor

So the goal here is to update ALL samples and docs that use : and replace with __? Just want to make sure I am following this.

@scottaddie
Copy link
Member

@isaac2004 That's my understanding.
\cc: @rachelappel

@rachelappel
Copy link
Contributor

@isaac2004 @scottaddie Yep. The first to change should be the configuration doc but at some point all docs that use this

@isaacrlevin
Copy link
Contributor

isaacrlevin commented Mar 21, 2018

@rachelappel @danroth27 @scottaddie Does it really make sense to update all the samples? The more I think about it, it may just make sense to update the note to something like

A colon (:) may also be used for hierarchical config keys, but may not work on all platforms. Double underscore (__) is supported by all platforms.

@scottaddie
Copy link
Member

This was addressed in #5876

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

No branches or pull requests

6 participants