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

Use PurePosixPath instead of Path for model name #70

Closed
wants to merge 1 commit into from
Closed

Use PurePosixPath instead of Path for model name #70

wants to merge 1 commit into from

Conversation

McGravel
Copy link
Collaborator

@McGravel McGravel commented Jul 20, 2021

Thanks Xbmann for accidentally coming across this error!

This should fix the UnicodeDecode exception if the user had \x in their model's path.

I don't know if PurePosixPath should be used elsewhere to fix any other backslashes, so I suppose this could be considered a hotfix of sorts.

I'm presuming this is only an issue on Windows since it likes to use backslashes in paths, unsure if functionality is affected on other platforms by this change.

If this fixes the error, then it may be worth considering what other paths should be changed to PurePosixPath

Example error given:
image

This *should* fix the UnicodeDecode exception if the user had `\x` in their model's path.
@McGravel
Copy link
Collaborator Author

McGravel commented Jul 20, 2021

the PurePosixPath does change the model's backslash to a forward one, so at least it matches the UI more...

However, the error still occurs; debugging seems to indicate that Popen makes the path with backslashes, even though the args variable appears to use forward ones as input?

image

subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

More info on what is happening on Windows: https://docs.python.org/3/library/subprocess.html#converting-argument-sequence

@McGravel
Copy link
Collaborator Author

This issue isn't what I was expecting, so i'll close this for now and consider making an Issue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant