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.

Sign up for GitHub

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

Jump to bottom

Fix duplicated path.Contains check and other checks in Path.GetFullPath #55373

Merged
merged 9 commits into from
Jul 24, 2021
Merged

Fix duplicated path.Contains check and other checks in Path.GetFullPath #55373

merged 9 commits into from
Jul 24, 2021

Conversation

steveberdy
Copy link
Contributor

@steveberdy steveberdy commented Jul 8, 2021 •
edited
Loading

Summary

Refactors code to prevent duplicate checks on paths in the Path.GetFullPath methods.

Fixes #54993

@ghost
Copy link

ghost commented Jul 8, 2021

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

Issue Details
Author: steveberdy
Assignees: -
Labels:

area-System.IO
Milestone: -

@steveberdy steveberdy changed the title Duplicated path.Contains('\0') check Fix duplicated path.Contains('\0') check in Path.GetFullPath Jul 8, 2021
@steveberdy steveberdy changed the title Fix duplicated path.Contains('\0') check in Path.GetFullPath Fix duplicated path.Contains check and other checks in Path.GetFullPath Jul 9, 2021
@steveberdy
Change helper method to internal
ce6bfd2 
Switched method from a private protection level to an internal protection level. Also removed trailing whitespace.
@runfoapp runfoapp bot mentioned this pull request Jul 10, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Jul 13, 2021
Closed
@steveberdy
Copy link
Contributor Author

@stephentoub Please review

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
jeffhandley
jeffhandley approved these changes Jul 24, 2021
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Nit suggestion on renaming the method. I'll go ahead and commit the suggestions and merge once checks rerun/pass.

src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated Show resolved Hide resolved
@jeffhandley jeffhandley self-assigned this Jul 24, 2021
@steveberdy
Copy link
Contributor Author

Thank you @jeffhandley!

@jeffhandley jeffhandley merged commit 003e09e into dotnet:main Jul 24, 2021
@karelz karelz mentioned this pull request Jul 28, 2021
Closed
@steveberdy steveberdy mentioned this pull request Jul 29, 2021
Merged
@adamsitnik adamsitnik added this to the 6.0.0 milestone Aug 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iSazonov iSazonov iSazonov left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

Assignees

@jeffhandley jeffhandley

Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Duplicated path.Contains('\0') check
5 participants
@steveberdy @jeffhandley @iSazonov @terrajobst @adamsitnik