Skip to content

Conversation

@breisfeld
Copy link
Contributor

On Python 3+, the str type has no method decode, so include try/except clauses.

@spderosso
Copy link
Member

Thanks for your PR! Were you getting any errors? On Python 3+ we get bytes back from sh (not str) so the call to decode should work:

Python 3.5.2 (default, Sep 28 2016, 18:08:09) 
[GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.38)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from sh import ls
>>> type(ls().stdout)
<class 'bytes'>

We should probably add a comment in the stdout/stderr functions to document this subtlety though

@breisfeld
Copy link
Contributor Author

breisfeld commented Nov 28, 2016

I am using the Windows version of gitless that makes use of pbs instead of sh.
I don't know if this is the cause, but for me p.stdout returns a str type.

@spderosso
Copy link
Member

That's good to know. That might very well be the cause.

@cougarhawk
Copy link

cougarhawk commented Aug 17, 2017

I also ran into this same problem myself with the Windows version of gitless. The root cause is that the pbs 0.110 python package is not compatible with the changes introduced to the str class in Python 3+.

On my machine, I modified the pbs.by contents directly to make gitless work on my Windows environment.

File C:\Program Files\Python36\Lib\site-packages\pbs.py:

220c220,221
<         return self._stdout.decode("utf8", "replace")
---
>         return self._stdout
> #        return self._stdout.decode("utf8", "replace")
225c226,227
<         return self._stderr.decode("utf8", "replace")
---
>         return self._stderr
> #        return self._stderr.decode("utf8", "replace")

Sincerely,
Al

@cougarhawk cougarhawk mentioned this pull request Aug 19, 2017
@embs
Copy link
Contributor

embs commented Dec 1, 2017

Relates to #146

@spderosso
Copy link
Member

Should I go ahead and merge this? Does this fix the problems on Windows + Python 3? It seems to be a no-op if you are not on Windows, does it break things on Windows + Python 2?

Now that we have appveyor setup we could have it run the checks and see if it breaks the build. @breisfeld could you merge with master? that should rerun the checks. Alternatively, I can merge it and revert the change if it breaks the build.

Also, I've changed the license to MIT since you've opened the PR, are you ok with contributing your change under the new license?

Thank you!

@breisfeld
Copy link
Contributor Author

I don't think that I have the required permission to do a merge. Perhaps you should do it.
I am fine with contributing my change under the MIT license.

@breisfeld breisfeld closed this Feb 12, 2018
spderosso added a commit that referenced this pull request Feb 12, 2018
spderosso added a commit that referenced this pull request Feb 12, 2018
@spderosso
Copy link
Member

done! thanks

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.

4 participants