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

Add support for .xz / lzma #260

Closed
wants to merge 3 commits into from
Closed

Add support for .xz / lzma #260

wants to merge 3 commits into from

Conversation

vmarkovtsev
Copy link
Contributor

Signed-off-by: Vadim Markovtsev [email protected]

Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition! 👍 for tests.

Can you update the README too, so it's clear we support this (obscure, at least to me) format?

Let's clear up the desired py2/py3 behaviour. I'm not a fan of smart_open behaving differently on the same file.

smart_open/smart_open_lib.py Outdated Show resolved Hide resolved
Signed-off-by: Vadim Markovtsev <[email protected]>
Signed-off-by: Vadim Markovtsev <[email protected]>
@vmarkovtsev
Copy link
Contributor Author

@piskvorky CI passed, README updated, py2.7 handled. PTAL

@piskvorky
Copy link
Owner

Thanks for the swift resolution!

@mpenkov WDYT?

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.

I read through it and left you some comments. Most of them are about improving the tests. Please have a look and let me know.

def test_http_xz(self):
"""Can open xz via http?"""
test_string = b'Hello World Compressed.'
#
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with the text in this TODO. We should avoid unnecessary file IO (create temporary file, write to file, read from file, delete file) in unit tests, especially when the alternative is cleaner and simpler (encode to a io.BytesIO directly).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been copied from test_http_bz. I understand that you wish to refactor the tests, but the goal of this PR is different. I will submit a refactoring PR separately, ok?

smart_open/tests/test_smart_open.py Show resolved Hide resolved
smart_open/tests/test_smart_open.py Show resolved Hide resolved
smart_open/tests/test_smart_open.py Show resolved Hide resolved
README.rst Show resolved Hide resolved
@vmarkovtsev
Copy link
Contributor Author

Sorry for the mess, the work continues in #262 as I accidentally removed my fork.

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.

3 participants