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 a SplitPath unit test exercising Windows paths with drive letters #13246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nlebeck
Copy link
Contributor

@nlebeck nlebeck commented Dec 27, 2024

The goal of this PR is to test Windows-specific behavior that is working as intended for SplitPath.

There are some behaviors of the current implementation that this PR does not test. For example:

  • A Windows path like "C:/dir/some_file.txt" parses fine on non-Windows platforms, with the same output.
  • An invalid path like "C:/malformed_dir/:some_file.txt" parses fine on Windows, with the output path being "C:/malformed_dir/:".

I referenced this page to figure out what exactly qualifies as a valid Windows path: https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats.

@BhaaLseN
Copy link
Member

I wonder if leaving the test empty is the way to go (vs. wrapping the whole test in the #ifdef instead).

@nlebeck
Copy link
Contributor Author

nlebeck commented Dec 27, 2024

I wonder if leaving the test empty is the way to go (vs. wrapping the whole test in the #ifdef instead).

I switched to wrapping the whole test in the #ifdef. (I'm not sure what the best convention is, and I don't have any preference.)

I also deleted and retyped the comment line that the linter was unhappy with. I couldn't see any difference in the diff, but maybe some unprintable character snuck in.

The OSX build failure on the latest version seems unrelated to this PR (dolphin.dmg: The timestamp service is not available.). But I don't know much about OSX development...

@JosJuice
Copy link
Member

I also deleted and retyped the comment line that the linter was unhappy with. I couldn't see any difference in the diff, but maybe some unprintable character snuck in.

There was a trailing space.

@BhaaLseN
Copy link
Member

The OSX build failure on the latest version seems unrelated to this PR (dolphin.dmg: The timestamp service is not available.). But I don't know much about OSX development...

That's an intermittent issue, nothing you need to worry about (for now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants