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

Fix default value description for URL #321

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

Frizlab
Copy link
Contributor

@Frizlab Frizlab commented Jun 1, 2021

At first I noticed a crash when running the tests of this project. It was due to weird chars in my CWD.
This lead me to discover a bug where the default value description of a URL checks for the absoluteString value of itself instead of the path + whether it is a file URL!

Note: I did not add a test specifically, I don’t really know how this would be tested. Maybe create a folder with a weird name and check everything is good? Also a test with an https URL as default w/ a path being the same as current CWD?
Tell me, I’ll do them if needed.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@kylemacomber
Copy link
Contributor

After a quick skim of the documentation, it looks like this might actually be a Foundation bug. FileManager.currentDirectoryPath is defined as returning a type String, but it appears possible it will return nil. It should probably be defined as returning String?.

@Frizlab
Copy link
Contributor Author

Frizlab commented Jun 3, 2021

@kylemacomber I don’t think so. Maybe it’s a separate issue, but unrelated to the one I reported.

URL(string: FileManager.default.currentDirectoryPath) is likely to fail, and is definitely not supposed to be used to create a file URL.
It might not return nil if by a lucky chance the current directory path happens to be a valid URL (e.g. a path with no space in it), but 1/ the returned URL will never be a file URL, and 2/ if the current path contains a space or any character not allowed in a URL the init will definitely return nil.

The correct way to init a file URL is URL(fileURLWithString: "/path/to whatever"), which will never return nil in addition to correctly returning a file URL.

EDIT: And of course, absoluteString is not the path of a URL. For a file URL it could yield something like file:///path/to%20whatever for instance.

@kylemacomber
Copy link
Contributor

@swift-ci please test

@natecook1000 natecook1000 merged commit 91fbf38 into apple:main Jun 4, 2021
@Frizlab Frizlab deleted the fix_test_crash branch June 4, 2021 15:12
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.

None yet

3 participants