-
Notifications
You must be signed in to change notification settings - Fork 339
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 ValueConverterExtension to allow for comparison of Nullable types to corresponding underlying types #1307
Conversation
Updated failing test in BoolToObjectConverterTests.cs
…sion.shared.cs, added unit tests to IsStringNotNullOrEmptyConverterTests.cs
@dotnet-policy-service agree |
src/CommunityToolkit.Maui/Extensions/ValueConverterExtension.shared.cs
Outdated
Show resolved
Hide resolved
src/CommunityToolkit.Maui/Extensions/ValueConverterExtension.shared.cs
Outdated
Show resolved
Hide resolved
…undant code and checks
Could you please add Sample? The other changes look good. |
Good call man, thank you! Just some questions before this PR gets merged:
Thanks man! |
@brminnick what do you think? |
I always say that the more tests the better! Bruce, if you're able to add similar unit tests for the other converters, that'd be incredible 💯 |
…verterTests.cs, IsStringNullOrEmptyConverterTests.cs and IsStringNullOrWhiteSpaceConverterTests.cs
Thanks @brminnick & @VladislavAntonyuk! |
Awesome!! Thanks @BruceTheBrick! @VladislavAntonyuk - I won't be able to review this until next week, but if you happen to have the time for one more review, we could merge this and fold it into the next week's v5.3.0 release! And no worries if not! We can release it in v6.0.0 next month. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @BruceTheBrick!!
Apologies - It turns out I was wrong in my previous review.
I tightened up the logic in ValueConverterExtension.ValidateTargetType()
to ensure that we're only enabling Nullable Value Types for TTo
and not TFrom
(mostly because I'm unsure if that would be a breaking change).
I added your new sample to IsStringNullOrEmptyConverterPage
as well.
Thanks for all your work there @brminnick, I really appreciate it! |
This PR aims to resolve the issues outlined in #1300
and align the behavior to comparable converters from Xamarin.Forms.