Closed bollwyvl closed 8 years ago
LGTM too... but let's wait for @malev too before merging...
Just a tiny comment @bollwyvl
Replied to the above comments, had to expand outdated diff. thanks for the feedback!
Looks good! I'm guessing that if you merge this one you won't need to merge #4
right. I started rebasing with the integration tests, which I plan to talk about at the code review...
On Thu, Jan 21, 2016 at 11:26 AM Marcos Vanetta notifications@github.com wrote:
Looks good! I'm guessing that if you merge this one you won't need to merge #4 https://github.com/Anaconda-Server/nb_anacondacloud/pull/4
— Reply to this email directly or view it on GitHub https://github.com/Anaconda-Server/nb_anacondacloud/pull/5#issuecomment-173624124 .
Merging this one as well... I like the incremental updates solving different issues in different PRs... no need to rebase...
Fixes #2.
This uses
BytesIO
to avoid the Unicode hashing issue. Also tried using binary tempfiles, etc. but this was a smaller change.This also has #4 in it, too... shouldn't cause too many issues.