-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
@@ -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(), |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same! TIL. nice :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noklam Yes I tested with select environment command to 'local', after changing, its now navigate to |
There was a problem hiding this 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.
There was a problem hiding this 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.
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 theas_uri()
method for resolving file URIs in_get_param_location
,_query_catalog
, andreference_location
methods.Refactored the
references
method to usePath.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)