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

Rename IsNullOrEmptyConverter to IsStringNullOrEmptyConverter #289

Merged
merged 7 commits into from
Feb 11, 2022

Conversation

brminnick
Copy link
Collaborator

@brminnick brminnick commented Feb 10, 2022

Description of Change

This PR renames the following converters:

  1. Rename IsNullOrEmptyConverter to IsStringNullOrEmptyConverter
    • Throw an InvalidArgumentException if object? value is not string
    • Use string.IsNullOrEmpty to evaluate value
  2. Rename IsNotNullOrEmptyConverter to IsStringNotNullOrEmptyConverter
    • Throw an InvalidArgumentException if object? value is not string
    • Use string.IsNullOrEmpty to evaluate value

Linked Issues

PR Checklist

@brminnick brminnick requested a review from bijington February 10, 2022 04:44
@brminnick brminnick linked an issue Feb 10, 2022 that may be closed by this pull request
10 tasks
bijington
bijington previously approved these changes Feb 11, 2022
Copy link
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @brminnick

@bijington bijington enabled auto-merge (squash) February 11, 2022 21:15
@bijington bijington merged commit d3abd5e into main Feb 11, 2022
@IeuanWalker
Copy link
Contributor

@brminnick Hi, think this change has broken my use case.

Was using this converter to check if double? was null or not, but now I'm getting a InvalidCastException

image

Any chance of a converter to check if an object? is null or not. I'm happy to do a PR for it.

@bijington
Copy link
Contributor

@IeuanWalker I suspect we will need an IsNullConverter and IsNotNullConverter given this change as you have pointed out. I don't think it would need to be IsObjectNullConverter but I am open to opinions here.

I don't know what the correct approach is here in terms of opening a proposal to cover this work or not. I shall delegate to the rest of the team on this one: @jfversluis @VladislavAntonyuk @pictos @brminnick

@VladislavAntonyuk
Copy link
Collaborator

@IeuanWalker I suggest to create a discussion. The new control by its name can be used for String only.
As an alternative you can use EqualConverter and check if value is null.
But in discussion we can discuss if we need a new IsNullConverter or we replace casting with System.Convert or we leave it as it is.

@IeuanWalker
Copy link
Contributor

@VladislavAntonyuk will open a discussion now.

I tried using the NotEqualConverter but doesn't seem to work for null checking

@pictos pictos deleted the Rename-IsNullOrEmptyConverter branch February 22, 2022 18:15
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] IsStringNullOrEmpty Converters
4 participants