Skip to content

Conversation

@WeatherGod
Copy link

Discovered this as I was porting a project over to py3k. My tests had some round-trippers that were ending up as b"b'foobar'", which would fail my unit test. This happens in py3k when casting a bytes object into a string. Since paramiko does everything as bytes in py3k (as per the SFTP specs), this would result in bad results being read back in a round-trip scenario.

The existing tests didn't notice this because any tests that wrote to the content provider never read it back, and any tests that read stuff back only read back entries in the existing content provider fixture. which were are already string objects. I added a round-trip test that fails without this change and passes with it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.455% when pulling 8d7a323 on WeatherGod:do_binary into 547b1d3 on ulope:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.455% when pulling 8d7a323 on WeatherGod:do_binary into 547b1d3 on ulope:master.

@WeatherGod WeatherGod mentioned this pull request Oct 12, 2018
@lorenyu
Copy link

lorenyu commented Sep 5, 2019

@ulope @WeatherGod can we merge this fix and re-publish the package? I ran into this issue as well. Looks like others have as well (e.g. januarytech@3635846).

@ulope
Copy link
Owner

ulope commented Sep 13, 2019

Sorry for being so slow reacting to PRs here.

I'll look into this on the weekend and release a new version then.

@ulope ulope force-pushed the master branch 13 times, most recently from bfaaa6d to 95a3bc8 Compare September 16, 2019 13:02
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit ef0817e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 98.4% (0.1% change).

View more on Code Climate.

@ulope ulope merged commit 5c057f6 into ulope:master Sep 16, 2019
@ulope
Copy link
Owner

ulope commented Sep 16, 2019

Merged. Thanks for the fix, and again apologies for it having taken so long.

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