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

unzip with gz too #4883

Merged
merged 5 commits into from Apr 9, 2019
Merged

unzip with gz too #4883

merged 5 commits into from Apr 9, 2019

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Mar 31, 2019

Changelog: Feature: tools.get() and tools.unzip() now handle also .gz compressed files
Docs: conan-io/docs#1230

Close #4876

@tags: slow

@ghost ghost assigned memsharded Mar 31, 2019
@ghost ghost added the stage: review label Mar 31, 2019
@memsharded memsharded added this to the 1.15 milestone Mar 31, 2019
@memsharded memsharded assigned lasote and unassigned memsharded Mar 31, 2019
@ghost ghost assigned memsharded Mar 31, 2019
@memsharded memsharded removed their assignment Apr 1, 2019
@ghost ghost assigned memsharded Apr 1, 2019
Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

@memsharded
Copy link
Member Author

Docs omit? don't be lazy please: https://docs.conan.io/en/latest/reference/tools.html#tools-unzip

Yes, sorry, I did a (very) quick look at the docs, and thought the extensions were not detailed. Docs added.

import gzip
with gzip.open(filename, 'rb') as f:
file_content = f.read()
target_name = filename[:-3] if destination == "." else destination
Copy link
Contributor

Choose a reason for hiding this comment

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

Here the destination is expected to be a file (not a directory like in the other cases), the docstring for this function doesn't match. If given a directory this function will fail in the load with a IsADirectoryError. I'm not sure if we want to handle it better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated docstring.

I have tried passing a folder as argument, and it failed (py2, Win), with IOError: [Errno 13] Permission denied: 'mytemp'. I think in either case the error should be clear enough, catching and re-raising a ConanException is a bit overkill for this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, don't need to reraise, it was just about the docstring (it is a documented tool). Don't forget to update it: https://docs.conan.io/en/latest/reference/tools.html#tools-unzip 😉

@memsharded memsharded removed their assignment Apr 8, 2019
@lasote lasote merged commit 337cd72 into conan-io:develop Apr 9, 2019
@ghost ghost removed the stage: review label Apr 9, 2019
@memsharded memsharded deleted the feature/unzip_gz branch April 16, 2019 13:56
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.

None yet

3 participants