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 parsing of windows file paths into a URI #1611

Merged
merged 6 commits into from
Oct 21, 2021
Merged

fix parsing of windows file paths into a URI #1611

merged 6 commits into from
Oct 21, 2021

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 requested a review from natebosch October 21, 2021 16:03
@google-cla google-cla bot added the cla: yes label Oct 21, 2021
@jakemac53
Copy link
Contributor Author

I didn't add tests here but I did manually test - I have #1612 out separately to try and get better windows test coverage generally (we have tests for this they just don't run on windows).

@jakemac53
Copy link
Contributor Author

Ah crap this won't work because now the query params etc aren't allowed :(.

We might just end up needing to manually convert \ to / or something...

@jakemac53
Copy link
Contributor Author

Talked with @natebosch offline and we decided to just check for a ? in the path and only try to parse it as a URI in that case.

@jakemac53 jakemac53 merged commit 777bb15 into master Oct 21, 2021
@DanTup
Copy link
Contributor

DanTup commented Oct 21, 2021

we decided to just check for a ? in the path and only try to parse it as a URI in that case.

Is this documented? It could be nice if it goes down the URI route then fails to find the file and there's a \ in the path, make the message more helpful (eg. explicitly call out that when using query strings the paths must be URIs using forward slashes).

It could be easy for tool authors that don't use Windows to ship code not realising this and it only fail for their users, so having a really clear message might same some debugging.

@jakemac53
Copy link
Contributor Author

Is this documented? It could be nice if it goes down the URI route then fails to find the file and there's a \ in the path, make the message more helpful (eg. explicitly call out that when using query strings the paths must be URIs using forward slashes).

I did add an explicit message for that case - https://github.com/dart-lang/test/pull/1611/files#diff-dc50945ce4dcf38563b0434145591a7d37ff5007d90063c048ccefd867bcd9cdR183.

It probably wouldn't hurt to update the docs for the feature to call that out as well though.

@jakemac53 jakemac53 deleted the windows-uris branch October 21, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running tests from VS Code not working (Failed to load "test%5Cword_count_test.dart": Does not exist.)
3 participants