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

Reconsider allowing null value in Converter parameter in MudTextField and MudNumericField #6659

Open
2 tasks done
Enderlook opened this issue Apr 15, 2023 · 4 comments
Open
2 tasks done
Assignees
Labels
enhancement New feature or request wants to do a PR

Comments

@Enderlook
Copy link
Contributor

Enderlook commented Apr 15, 2023

Feature request type

Enhance component

Component name

MudTextField and MudNumericField

Is your feature request related to a problem?

When writing wrappers around MudTextField and MudNumericField, it's a bit frustrating that passing a null value to Converter parameter throws, instead of using the DefaultConverter<T>.
The odd thing about it is that if you don't assign this property (that is, you let the default value from the constructor), it does works, so as a consumer I would expect that cleaning this property would return the component to its original converter.

Describe the solution you'd like

I would like it to support passing null values to the property Converter in both MudTextField and MudNumericField components without throwing. Instead, it would use the original value it had at construction (that is: new DefaultConverter<T>()).

I haven't tried to do the actual modification yet, but I think it's just replacing:

_converter = converter ?? throw new ArgumentNullException(nameof(converter));

With _converter = converter ?? new DefaultConverter<T>().

_converter = value ?? throw new ArgumentNullException(nameof(value)); // converter is mandatory at all times

With _converter = value ?? new DefaultConverter<T>().

Also, as a small optimization a public static readonly DefaultConverter<T> Shared field could be added to DefaultConverter<T>, so instead of allocating a converter on each component, we can reuse the same instance.

And modifying documentation API to show they are nullable now.

Have you seen this feature anywhere else?

No response

Describe alternatives you've considered

When writing wrappers of these components for my projects, I could manually assign the new DefaultConverter<T>() to Converter if I don't have any specific converter and want to use the default one.

Pull Request

  • I would like to do a Pull Request

Code of Conduct

  • I agree to follow this project's Code of Conduct
@Enderlook Enderlook added enhancement New feature or request triage labels Apr 15, 2023
@github-actions
Copy link

👋 Thanks for wanting to do a PR @Enderlook !
We suggest that you wait for an answer from @MudBlazor/contribution-team @MudBlazor/core-team . Otherwise we can not guarantee that your PR will be merged. As the library grows, we have to be very strict what PRs we can accept.

@TDroogers
Copy link
Contributor

Hi @Enderlook have a look at issue #6535
@ScarletKuro i guess this issue will be solved with #6535

@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 20, 2023

Hi @Enderlook have a look at issue #6535 @ScarletKuro i guess this issue will be solved with #6535

Hi. Author wants to modify the behavior so that passing a null value would not result in an ArgumentNullException in the Converter parameter. The issue (#6535) you mentioned is just about documenting which fields, arguments, and properties can be null and which cannot. It does not change the actual behavior of the code.

@TDroogers
Copy link
Contributor

Sorry @Enderlook I was a bit to fast with my previous reply.

@henon henon removed the triage label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wants to do a PR
Projects
None yet
Development

No branches or pull requests

4 participants