Allow registration of DefaultConverters for non-standard types

MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.

http://mudblazor.com

MIT License

7.21k stars 1.18k forks source link

Allow registration of DefaultConverters for non-standard types #8799

Closed rafalmaciag closed 3 weeks ago

rafalmaciag commented 3 weeks ago

This is a tiny change that allows a developer to set up a global default converter that is used in bindings. With this change, one can setup global default converter for structs such as Point, Vector, etc, especially from 3rd party libraries.

Description

Code was added, not changed. It is backward compatible.

How Has This Been Tested?

--> A UT was added to test this small change. BindingConverterTests->GlobalConverterTests() --> docs project contains a component that tested new functionality: PointConverterExample

Type of Changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[x] Documentation (fix or improvement to the website or code docs)

Checklist

[x] The PR is submitted to the correct branch (dev).
[x] My code follows the code style of this project.
[x] I've added relevant tests.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 85.71429% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.05%. Comparing base (28bc599) to head (2304280). Report is 112 commits behind head on dev.

Files	Patch %	Lines
...or/Utilities/BindingConverters/DefaultConverter.cs	85.71%	0 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev #8799 +/- ## ========================================== + Coverage 89.82% 90.05% +0.22% ========================================== Files 412 420 +8 Lines 11878 12213 +335 Branches 2364 2409 +45 ========================================== + Hits 10670 10999 +329 + Misses 681 671 -10 - Partials 527 543 +16 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ScarletKuro commented 3 weeks ago

Thanks for the PR!

@henon I will let you decide on this one, considering that in maybe v8 we will rethink the whole converter system I don't think "global" converters will be a thing.

rafalmaciag commented 3 weeks ago

To be honest, I don't like the way it ended up, but this way, it didn't require a lot of effort. Few alternatives I've considered:

IParsable -- but requires and notating everywhere, so that compiler don't trim its methods names.
DI -- this would be most likely preferable, MudBlazor already has converters at constructors, but for some reason (I don't know) don't use DI to resolve them.

henon commented 3 weeks ago

I thought quite a bit about this. We have a default converter which is already part of our global default value infrastructure. Making it customizable for non-standard types makes sense, otherwise you would have to question the existence of a universal default converter as a whole, right? I mean you have default conversion for standard types but not for other types. And as a user you might even want to override how standard types are converted.

I am accepting this PR for v7 but we'll probably change how to register default conversion functions / default converters in v8. Or if we face implementation challenges or find that it introduces too much complexity with our new converter API we might also choose to remove the capability in v8. Let's see.

rafalmaciag commented 3 weeks ago

Awesome, thanks.