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

Clarify type binding capabilities of the configuration binder #17151

Closed
jackhorton opened this issue Feb 28, 2020 — with docs.microsoft.com · 9 comments
Closed

Clarify type binding capabilities of the configuration binder #17151

jackhorton opened this issue Feb 28, 2020 — with docs.microsoft.com · 9 comments
Assignees
Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue
Milestone

Comments

Copy link
Contributor

Upon installing FxCop analyzers and being presented with the "do not store url properties as strings" warning, I learned that I could just make the property of the class I was binding my configuration to be a System.Uri and valid Uris would magically work. Separately, a colleague suggested we stop using "MyTimespanConfigMinutes=5" for TimeSpan-bound configuration and instead use "00:05:00" based on the guidance here: https://blog.ploeh.dk/2019/11/25/timespan-configuration-values-in-net-core/. This also worked magically. Having two surprisingly magic cases of non-POCO bindings, I investigated more and found that you can actually bind to anything that supports converting from a string due to the use of System.ComponentModel.TypeDescriptor.GetConverter(propertyType) here: https://github.com/dotnet/extensions/blob/master/src/Configuration/Config.Binder/src/ConfigurationBinder.cs#L508-L513.

I think this is extremely useful information that should be documented so that people see that they can use TimeSpan, Uri, and many others out of the box (its also useful to know that this kind of conversion is used, rather than IConvertable conversion or the System.CommandLine approach of allowing anything with a constructor that takes a single string). Finally, https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2013/ayybcxe5(v=vs.120) is linked to by the TypeConverter docs and its a useful resource, but it should probably be modernized and moved out of the VS 2013 docs into somewhere more discoverable.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@guardrex
Copy link
Collaborator

guardrex commented Feb 28, 2020

Hello @jackhorton ... I suppose that we could describe in a little more detail here which ones are supported OOB. Note that I'm going to have to backlog this quite a bit out because we're all 🏃😅 for the foreseeable future with high priority work on a number of fronts. I can't really say when I'll be free to work this one.

WRT TypeConverter ... that's more of a custom model binding subject. I'm not very familiar with what we have, since I've only glanced at and lightly touched https://docs.microsoft.com/aspnet/core/mvc/advanced/custom-model-binding over the past few years. On a slightly related note, I do have a plan to investigate adding TypeConverter content for Blazor binding (Creating Dynamic component with databinding from datatable (ADO.NET)). It might be best for you to look at the custom model binding topic and see if it covers what you're suggesting. If not, then mention your request on a new issue opened from the bottom of that topic. (It's all model binding for MVC.)

@guardrex guardrex self-assigned this Feb 28, 2020
@guardrex guardrex added Pri3 Priority 3 and removed ⌚ Not Triaged labels Feb 28, 2020
@guardrex guardrex added this to the Backlog milestone Feb 28, 2020
@guardrex guardrex changed the title No guidance on how to bind non-POCO types Clarify type binding capabilities of the configuration binder Feb 28, 2020
@jackhorton
Copy link
Contributor Author

As far as I could tell, the configuration binding code and MVC model binding code were completely different (perhaps they shouldnt be, but thats definitely a separate issue). Since there are already sections on the page for binding configuration to arrays and POCOs, I was just hoping to get a blurb slotted in about binding to arbitrary types using type converters. I would be willing to make a PR to add such a blurb, I just thought I would make the issue first to ensure I wasn't missing any context.

@guardrex
Copy link
Collaborator

Yes, they are different. I just meant that that's the context where TypeConverter normally comes up, and I really don't know/recall what that doc says or shows on the matter. I'm 🏃 right now with high priority work, so I didn't even look.

a blurb slotted in about binding to arbitrary types using type converters

Maybe, but I think you should paste about what you're suggesting in here. If your suggestion is just to mention it, I don't think that's enough to be of any use to anyone. I have a feel'in that an example is going to be required for something that doesn't bind OOB ... such an example might make this long and complex and thus be better suited for a community blog post as an advanced scenario.

@Rick-Anderson Rick-Anderson removed the Pri3 Priority 3 label Apr 13, 2020
@Rick-Anderson
Copy link
Contributor

@serpent5 please review.

@serpent5
Copy link
Contributor

There's an open issue being tracked in #16319 for covering how this works with custom model-binding.

Finally, https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2013/ayybcxe5(v=vs.120) is linked to by the TypeConverter docs and its a useful resource, but it should probably be modernized and moved out of the VS 2013 docs into somewhere more discoverable.

"Configuration in ASP.NET Core" and "Custom Model Binding in ASP.NET Core" could both link to this, whether it's that version or a newer, better-placed version. It doesn't seem like that topic should live within the docs here, as it's not part of ASP.NET Core.

@Rick-Anderson
Copy link
Contributor

@serpent5 Thanks

@jackhorton
Copy link
Contributor Author

That issue discusses the ASP.NET MVC model binder, this issue is for the IOptions binder. They are different implementations and you can easily use Microsoft.Extensions.Options without touching ASP.NET (which is what we are doing). I dont think this issue should be closed.

@serpent5
Copy link
Contributor

Maybe, but I think you should paste about what you're suggesting in here. If your suggestion is just to mention it, I don't think that's enough to be of any use to anyone. I have a feel'in that an example is going to be required for something that doesn't bind OOB ... such an example might make this long and complex and thus be better suited for a community blog post as an advanced scenario.

I'd echo this from @guardrex. Just stating that TypeCoverters are used for certain types, but without covering how to write and integrate your own, might leave things incomplete. It could also prove difficult to try and provide an exhaustive list of what's supported OOB here, especially as it's going to overlap with the custom model-binding topic. For example, if this configuration topic covers how a timespan can be specified in specific formats, that information would also be the same for model-binding.

For me, I see a relationship where this topic and the model-binding topic both highlight that TypeConverters are used and link to a common topic that covers the details of what is supported and how to roll your own. I can't speak for whether that's something this docs team has the capacity for, though.

@guardrex
Copy link
Collaborator

guardrex commented Apr 13, 2020

I know that I'd like one example in the Blazor docs for a use case that a dev asked about ... binding directly to a datatable, which should demo the general approach in such Razor component scenarios. It's a low priority issue tho, so I wouldn't get to that one for quite a long while. I have an 🌊 ocean of work 🌊 ahead of me. 🏃🏃🏃🏃🏃🏃🏃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
None yet
Development

No branches or pull requests

5 participants