Skip to content

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Sep 16, 2017

Internal review URL
Fixes #2409
Fixes #1382

🎸 Jam'in! 🎷

Review site topic

This turned out to be a considerable overhaul of the CommandLine config provider section:

  • The whole topic (big IMHO here) is toooooo console app-ish. We want to maintain the basic config explanations. What's a 👦 to do? 💡 ⚡ 3RD TAB! ⚡ 💡 The "Basic Configuration" is a tab with the basics (as the topic currently embraces in each of its samples). We can have the 2.x and 1.x bits in tabs 2 and 3 showing how the config applies to web apps. The reader isn't left wondering how it looks in a web app setup and then annoyed by the need to go look at other topics. If u don't like this, then I recommend we pull that tab 1 content out above the tab control and create a new section for the web app config (tabs).
  • I attempt to clarify the argument format situation. Lists didn't look good, so I went with tables.
  • I made an effort to explain out what's happening in the switch mappings, including a table of what's in the dictionary.
  • @rachelappel, you may need to gut this or make large changes to it when you come to this topic for 2.0 updates. It's cool. In the meantime tho, this fixes the two open issues pertaining to this section, and closing two issues feels really good.
  • Of course, this needs a good dev team 👁️ to make sure I have the logic and behaviors are correct.

cc/ @danroth27

@dnfclas
Copy link

dnfclas commented Sep 16, 2017

@guardrex,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@guardrex
Copy link
Collaborator Author

guardrex commented Sep 16, 2017

The build doesn't like it too much that I add a readme.md. They (or MS) must have that locked down a little bit, so I'm going to remove that file. The contents are simply:

The code in this folder is used for snapshots of code in the Configuration topic at: https://docs.microsoft.com/aspnet/core/fundamentals/configuration

@Rick-Anderson
Copy link
Contributor

@guardrex can you add yourself as an author?

@guardrex
Copy link
Collaborator Author

@Rick-Anderson Done.

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Very nice improvements!

@guardrex
Copy link
Collaborator Author

Thanks @HaoK!

@guardrex
Copy link
Collaborator Author

@Rick-Anderson If you don't want the 1.x tab there, we'd still need to work out how to show the reader the use of ConfigurationBuilder versus using the baked-in config bits of CreateDefaultBuilder of 2.x.

Do you want me to take a stab at getting the 1.x part (tab) out while keeping the content? ... imo it would be good to keep all of the content here but re-organize it to drop the 1.x tab. I can have it done by tomorrow morning.

@Rick-Anderson
Copy link
Contributor

@guardrex keep the 1.x tab. For new content updates, we'll drop the 1.x content and just update docs for 2.x (and point to https://github.com/aspnet/Docs/blob/master/aspnetcore/common/_static/9-25-17.pdf)

@Rick-Anderson
Copy link
Contributor

@guardrex can I merge or do you need more work? We can leave the 1.x tabs.

@guardrex
Copy link
Collaborator Author

@Rick-Anderson Thanks. I'm cool. Go ahead. I was only concerned about the 1.x tab situation.

@Rick-Anderson Rick-Anderson merged commit 03911a0 into dotnet:master Sep 25, 2017
@guardrex guardrex deleted the guardrex/config-cmdline-args-update branch September 25, 2017 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants