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

Allow registration of DefaultConverters for non-standard types #8799

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

rafalmaciag
Copy link
Contributor

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)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

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

@github-actions github-actions bot added docs Related to docs enhancement New feature or request PR: needs review labels Apr 23, 2024
Copy link

codecov bot commented Apr 23, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro requested a review from henon April 23, 2024 23:29
@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 23, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

henon commented Apr 24, 2024

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.

@henon henon changed the title Global Default Converter Allow registration of DefaultConverters for non-standard types Apr 24, 2024
@henon henon merged commit 15be0f5 into MudBlazor:dev Apr 24, 2024
4 checks passed
@rafalmaciag
Copy link
Contributor Author

Awesome, thanks.

biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
…azor#8799)

Co-authored-by: Rafał Maciąg <rafal.maciag@modelingevolution.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Related to docs enhancement New feature or request PR: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants