Skip to content

Fix: decode lsp url for workspace_diagnostics_picker - #6016

Merged
archseer merged 1 commit into
helix-editor:masterfrom
erasin:diagnostic_workspace_url
Feb 16, 2023
Merged

Fix: decode lsp url for workspace_diagnostics_picker#6016
archseer merged 1 commit into
helix-editor:masterfrom
erasin:diagnostic_workspace_url

Conversation

@erasin

@erasin erasin commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

erasin commented Feb 16, 2023

Copy link
Copy Markdown
Contributor Author

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

thanks for review.

@pascalkuthe pascalkuthe left a comment

Copy link
Copy Markdown
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants