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

Parse the original prompt for cwd and env names #1070

Merged
merged 1 commit into from Aug 15, 2016

Conversation

@jankatins
Copy link
Contributor

jankatins commented Aug 9, 2016

clink.get_cwd() is returning a string which is differently encoded than what
clink.prompt.value expects. This results in garbled path names if the path
condains non-ASCII chars. The (arguable hacky) solution is to parse the old
prompt for the current directory (which breaks if the user sets a PROMPT env var
which is incompatible to the regex used here...).

Also parse out a environment name set by systems like virtualenv or conda: this
could be done more specifically by targeting each such system and using the
usually set environment variable but this would mean that we would have to do
that for each and every such system out there and that is probably not a sane
idea...

Closes: #1054 (garbled prompt)
Closes: #1057 #1056 (environment issues)

Example session:

C:\Users\jschulz
λ activate py35

C:\Users\jschulz
(py35) λ cd c:\temp\ü\(hallo)\

c:\temp\ü\(hallo)
(py35) λ

A "better" solution would be proper unicode support in clink: mridgers/clink#415

clink.get_cwd() is returning a string which is differently encoded than what
clink.prompt.value expects. This results in garbled path names if the path
condains non-ASCII chars. The (arguable hacky) solution is to parse the old
prompt for the current directory (which breaks if the user sets a PROMPT env var
which is incompatible to the regex used here...).

Also parse out a environment name set by systems like virtualenv or conda: this
could be done more specifically by targeting each such system and using the
usually set environment variable but this would mean that we would have to do
that for each and every such system out there and that is probably not a sane
idea...
@jing2si

This comment has been minimized.

Copy link

jing2si commented Aug 15, 2016

nice work~

@jing2si jing2si mentioned this pull request Aug 15, 2016
@codesboy

This comment has been minimized.

Copy link

codesboy commented Aug 15, 2016

@janschulz @jing2si
Good job! Buddy. ⊙o⊙

@Stanzilla

This comment has been minimized.

Copy link
Member

Stanzilla commented Aug 15, 2016

@janschulz do you still want to wait for a response to your clink issue or won't that change what is in this PR?

@jankatins

This comment has been minimized.

Copy link
Contributor Author

jankatins commented Aug 15, 2016

@Stanzilla IMO this is a nice shortterm fix for the problem. If there is a better way (=proper unicode support in clink) , I will add a new PR.

@Stanzilla Stanzilla merged commit 832c199 into cmderdev:master Aug 15, 2016
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.