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

#51371 removed CaseInsensitivePlatforms and CaseSensitivePlatforms parameters #64440

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

mkhamoyan
Copy link
Contributor

@mkhamoyan mkhamoyan commented Jan 28, 2022

#51371 Refactored casesesintive test cases to use IsCaseSensitiveOS and IsCaseInsensitiveOS.

@ghost
Copy link

ghost commented Jan 28, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Refactored casesesintive test cases to use IsCaseSensitiveOS and IsCaseInsensitiveOS.

Author: mkhamoyan
Assignees: mkhamoyan
Labels:

area-System.IO

Milestone: -

@mkhamoyan
Copy link
Contributor Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -224,8 +224,7 @@ public void WindowsWhiteSpaceAsPath_ReturnsFalse(string component)

}

[Fact]
[PlatformSpecific(CaseInsensitivePlatforms)]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsCaseInsensitiveOS))]
Copy link
Member

Choose a reason for hiding this comment

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

just for my education: what is the benefit of doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we had 2 ways to check "case sensitivity" having only one will make it it less confusing. And I kept boolean parameter because in some places only bool can be used.

@danmoseley
Copy link
Member

If you put the issue ID in the description then it becomes hyperlinked. You can also put "Fixes ..." if the merge should close the issue.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for explaining me why this change was needed.

@mkhamoyan mkhamoyan merged commit 8766a1c into dotnet:main Jan 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2022
@mkhamoyan mkhamoyan deleted the 51371_refactor_casesensitive_tests branch August 19, 2022 12:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants