-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add StringExtensions Unit Tests #14099
Conversation
# Conflicts: # src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Seems there are some extensions need to be removed (unused) or replaced by native APIs |
Strange!! the build failed for unrelated issue @MikeAlhayek any idea? |
} | ||
|
||
if (chars == null || chars.Length == 0) | ||
if (string.IsNullOrEmpty(source) || chars == null || chars.Length == 0) |
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.
This is a wrong logic
if (chars == null || chars.Length == 0)
{
return false;
}
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.
Why?!! the old logic was wrong, when source
is null
it should returns false
not true
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.
we'll that's different that the previous logic. It could be one of the reason the tests are failing.
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.
There was no test before, and that is why the test failed when I use the previous logic
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
Show resolved
Hide resolved
Do you want to get back to reviewing this, @MikeAlhayek? |
@MikeAlhayek Anything to add here or shall I merge this? |
In favor of #14068