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

Encoding DocumentUri.Path when converting DocumentUri to Uri #11574

Merged
merged 5 commits into from
Aug 29, 2023
Merged

Encoding DocumentUri.Path when converting DocumentUri to Uri #11574

merged 5 commits into from
Aug 29, 2023

Conversation

shenglol
Copy link
Contributor

@shenglol shenglol commented Aug 18, 2023 •
edited by microsoft-github-policy-service bot
Loading

This is a workaround for OmniSharp/csharp-language-server-protocol#1005.

Fixes #9466.

Microsoft Reviewers: Open in CodeFlow

@shenglol shenglol changed the title Encoding DocumentUri.Path Encoding DocumentUri.Path when converting DocumentUri to Uri Aug 18, 2023
@shenglol shenglol changed the title Encoding DocumentUri.Path when converting DocumentUri to Uri Encoding DocumentUri.Path when converting DocumentUri to Uri Aug 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2023 •
edited
Loading

Test Results (linux-musl-x64)

       33 files  ±0         33 suites  ±0   32m 18s ⏱️ - 4m 9s
10 409 tests ±0  10 373 ✔️ ±0  36 💤 ±0  0 ❌ ±0 
12 626 runs  ±0  12 590 ✔️ ±0  36 💤 ±0  0 ❌ ±0 

Results for commit d4090a6. ± Comparison against base commit 52fa1e2.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2023 •
edited
Loading

Test Results (win-x64)

       33 files  ±0         33 suites  ±0   29m 48s ⏱️ - 10m 15s
10 421 tests ±0  10 385 ✔️ ±0  36 💤 ±0  0 ❌ ±0 
12 637 runs  ±0  12 601 ✔️ ±0  36 💤 ±0  0 ❌ ±0 

Results for commit d4090a6. ± Comparison against base commit 52fa1e2.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2023 •
edited
Loading

Test Results (linux-x64)

       33 files  ±0         33 suites  ±0   31m 59s ⏱️ - 3m 23s
10 409 tests ±0  10 373 ✔️ ±0  36 💤 ±0  0 ❌ ±0 
12 626 runs  ±0  12 590 ✔️ ±0  36 💤 ±0  0 ❌ ±0 

Results for commit d4090a6. ± Comparison against base commit 52fa1e2.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2023 •
edited
Loading

Test Results (osx-x64)

       33 files  ±0         33 suites  ±0   1h 51m 5s ⏱️ + 17m 46s
10 413 tests ±0  10 377 ✔️ ±0  36 💤 ±0  0 ❌ ±0 
12 630 runs  ±0  12 594 ✔️ ±0  36 💤 ±0  0 ❌ ±0 

Results for commit d4090a6. ± Comparison against base commit 52fa1e2.

♻️ This comment has been updated with latest results.

@shenglol shenglol force-pushed the shenglol/fix-folder-name-encoding branch from 94c4be3 to 0ddb3bf Compare August 20, 2023 04:04
@anthony-c-martin
Copy link
Member

Have you weighed up this approach against using .ToString()?

Fiddle: https://dotnetfiddle.net/ReOvDS

@shenglol
Copy link
Contributor Author

Have you weighed up this approach against using .ToString()?

Fiddle: https://dotnetfiddle.net/ReOvDS

That's a good find. I haven't explored ToString(), but it seems to fix the issue as well. The difference is that ToString is a slightly more expensive operation, but it handles encoding for other components (although for file system paths, this might not be a concern). Do you think if it's worth changing the implementation?

davidcho23
davidcho23 reviewed Aug 24, 2023
View reviewed changes
src/Bicep.LangServer/Extensions/DocumentUriExtensions.cs
{
#pragma warning disable RS0030 // Do not use banned APIs
return documentUri
.With(new() { Path = documentUri.Path.Replace("%", "%25") })
Copy link
Contributor

@davidcho23 davidcho23 Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason why 25 specifically is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

%25 is the url-encoded string for the % character: https://www.w3schools.com/tags/ref_urlencode.ASP.

@shenglol shenglol merged commit e0ce74d into main Aug 29, 2023
@shenglol shenglol deleted the shenglol/fix-folder-name-encoding branch August 29, 2023 17:24
@shenglol shenglol mentioned this pull request Aug 29, 2023
Closed
@majastrz
Copy link
Member

@shenglol I haven't read through the PR completely, but do we have any remaining ToUri() calls left? If not, should we make that a banned symbol?

@shenglol
Copy link
Contributor Author

Yup, it's already a banned symbol.

@majastrz
Copy link
Member

Yup, it's already a banned symbol.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidcho23 davidcho23 davidcho23 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error in File Reference in file path containing %20
4 participants
@shenglol @anthony-c-martin @majastrz @davidcho23