-
Notifications
You must be signed in to change notification settings - Fork 794
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
potential fix for 5932 #6070
potential fix for 5932 #6070
Conversation
seems to work on my local machine |
So, CI passes on your local machine? |
Some test is being very picky about exactly which exceptions are raised when for badly formed file paths. Trying again. |
OK, this is now ready. I included some cleanup and some notes about the somewhat odd use of path normalization in that code (which I couldn't remove easily but I left some notes about how we might go about removing it) |
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.
I recommend changing the normalization function to use an explicit parameter annotation. Should we ever move to .NET Core or .NET Standard 2.1 in the future, this code will break due to the ReadOnlySpan<char>
overload: https://docs.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?view=netcore-2.2#System_IO_Path_IsPathRooted_System_ReadOnlySpan_System_Char__
Co-Authored-By: dsyme <[email protected]>
@cartermp Done, thanks |
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.
Great, LGTM. Should alleviate #5932 to the point where any further work would require a redesign, which is exactly where we want to be w.r.t. perf problems
Possible fix for perf problem #5932