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

Add StringExtensions Unit Tests #14099

Merged
merged 16 commits into from
Apr 13, 2024
Merged

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Aug 9, 2023

In favor of #14068

# Conflicts:
#	src/OrchardCore/OrchardCore.ContentManagement.Abstractions/Utilities/StringExtensions.cs
@hishamco hishamco marked this pull request as ready for review September 15, 2023 18:42
@hishamco hishamco requested a review from agriffard September 15, 2023 18:42
@hishamco
Copy link
Member Author

Seems there are some extensions need to be removed (unused) or replaced by native APIs

@hishamco
Copy link
Member Author

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)
Copy link
Member

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;
}

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

@hishamco hishamco requested a review from MikeAlhayek February 23, 2024 23:41
@hishamco hishamco requested a review from MikeAlhayek February 24, 2024 23:30
@Piedone
Copy link
Member

Piedone commented Mar 21, 2024

Do you want to get back to reviewing this, @MikeAlhayek?

@hishamco
Copy link
Member Author

hishamco commented Apr 12, 2024

@MikeAlhayek Anything to add here or shall I merge this?

@hishamco hishamco removed the request for review from agriffard April 12, 2024 23:35
@hishamco hishamco merged commit f59edd1 into main Apr 13, 2024
5 checks passed
@hishamco hishamco deleted the hishamco/string-extensions-tests branch April 13, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants