Skip to content

Fix: decode lsp url for workspace_diagnostics_picker #6016

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
Merged
merged 1 commit into from
Feb 16, 2023
Merged

Fix: decode lsp url for workspace_diagnostics_picker #6016

merged 1 commit into from
Feb 16, 2023

Conversation

erasin
Copy link
Contributor

@erasin erasin commented Feb 16, 2023

When I used CJK characters as the path, the lsp returned the encoded content, and the correct path would be displayed only after decoding.

dia2
WX20230216-174046

@archseer
Copy link
Member

This seems unnecessary since the url library already uses percent-encoding. It's probably url.path() that re-encodes the path

@archseer
Copy link
Member

Yup: "Return the path for this URL, as a percent-encoded ASCII string."

@erasin erasin force-pushed the diagnostic_workspace_url branch from dca658a to 125b34f Compare February 16, 2023 14:10
@erasin erasin force-pushed the diagnostic_workspace_url branch from 125b34f to 45d9121 Compare February 16, 2023 14:12
@erasin
Copy link
Contributor Author

erasin commented Feb 16, 2023

Remove percent-encoding,The url.to_file_path() has return decode path.

thanks for review.

pascalkuthe
pascalkuthe approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

This LGTM now 👍 We simply used the wrong function here. We already use to_file_path everywhere else.

@pascalkuthe pascalkuthe added C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer. A-helix-term Area: Helix term improvements labels Feb 16, 2023
@archseer archseer merged commit b7fb52d into helix-editor:master Feb 16, 2023
@erasin erasin deleted the diagnostic_workspace_url branch February 16, 2023 14:48
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
@erasin @wes-adams
fix: decode lsp url for workspace_diagnostics_picker (helix-editor#6016)
3c67a20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalkuthe pascalkuthe pascalkuthe approved these changes

Assignees
No one assigned
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erasin @archseer @pascalkuthe