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 for windows path issue #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix for windows path issue #150

wants to merge 1 commit into from

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Nov 11, 2024

Description

Resolves #102

This pull request includes several updates to the bundled/tool/lsp_server.py file, focusing on improving the way file URIs are resolved and refining the handling of pipeline files. The most important changes are as follows:

Development notes

  • Updated the Location object creation to use the as_uri() method for resolving file URIs in _get_param_location, _query_catalog, and reference_location methods.

  • Refactored the references method to use Path.read_text() for reading pipeline files, which includes handling potential I/O and Unicode errors.

QA notes

Used Parallels Desktop Pro to test on Windows.

Gif (Windows)

windows-1

Signed-off-by: Jitendra Gundaniya <[email protected]>
@jitu5 jitu5 requested a review from noklam November 11, 2024 10:44
@jitu5 jitu5 self-assigned this Nov 11, 2024
@jitu5 jitu5 added bug Something isn't working Python Pull requests that update Python code labels Nov 11, 2024
@@ -250,7 +250,7 @@ def _get_param_location(server: KedroLanguageServer, word: str) -> Optional[Loca
continue

location = Location(
uri=f"file://{parameters_file.resolve().as_posix()}",
uri=parameters_file.resolve().as_uri(),
Copy link
Contributor

@noklam noklam Nov 11, 2024

Choose a reason for hiding this comment

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

>>> path
PosixPath('C:\\Window\\sysm\\some_file.csv')
>>> path.resolve()
PosixPath('/Users/Nok_Lam_Chan/C:\\Window\\sysm\\some_file.csv')
>>> path.resolve().as_uri()
'file:///Users/Nok_Lam_Chan/C%3A%5CWindow%5Csysm%5Csome_file.csv'

TIL: Brilliant!

Choose a reason for hiding this comment

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

same! TIL. nice :)

@jitu5 jitu5 marked this pull request as ready for review November 11, 2024 10:59
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

thanks @jitu5, the fix looks good for me. One more thing to check: #53

Though the issue is about settings, but from the screenshot it looks like a Window issue with the select environment command. Have you check that as well?

@jitu5
Copy link
Contributor Author

jitu5 commented Nov 11, 2024

thanks @jitu5, the fix looks good for me. One more thing to check: #53

Though the issue is about settings, but from the screenshot it looks like a Window issue with the select environment command. Have you check that as well?

@noklam Yes I tested with select environment command to 'local', after changing, its now navigate to local

windows-3

Copy link

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

I don't have windows desktop to test it. But based on the issue around Windows Path, and looks like Nok tested it already, it looks good to me.

@noklam noklam self-requested a review November 11, 2024 17:58
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Approved in principle, let's wait a bit for the user to come back. Otherwise we can merge this in two days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension not working properly for Windows
3 participants