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 file opener brotli.open for Python #806

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

youknowone
Copy link

The goal of this PR is adding brotli.open like gzip.open.

The patch is not completed yet - but the basic function compress/decompress is done as proof of concept. file_test.py is testing it.

Before revising more, I want to know if this project is interested in this patch or not.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@youknowone
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@AppVeyorBot
Copy link

Build brotli 1.0.0#1481 failed (commit 154f35a988 by @youknowone)

@youknowone youknowone force-pushed the open branch 2 times, most recently from 4e3cee0 to 1befb25 Compare May 12, 2020 23:08
@AppVeyorBot
Copy link

Build brotli 1.0.0#1482 failed (commit 7debde6096 by @youknowone)

@AppVeyorBot
Copy link

Build brotli 1.0.0#1483 failed (commit b3d2289b67 by @youknowone)

@eustas
Copy link
Collaborator

eustas commented May 15, 2020

Looks interesting.
Unfortunately, can't find the mentioned "Andrew Kuchling's minigzip.py". Could you share a link to it?

@youknowone
Copy link
Author

youknowone commented May 16, 2020

I copied it from CPython stdlib. So actually I didn't see that file. This is the CPython file: https://github.com/python/cpython/blob/master/Lib/gzip.py#L6

@google-cla
Copy link

google-cla bot commented Aug 2, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@youknowone
Copy link
Author

I splited CPython contributed part as a seperated commit but it causes CLA warning. what should i do in this case?

@AppVeyorBot
Copy link

Build brotli 1.0.0#1501 failed (commit f6b351df41 by @youknowone)

@youknowone
Copy link
Author

youknowone commented Aug 2, 2020

By needs, I released this one as a seperated project at this time

Because dependencies are using Python3 for now, it needs a few tasks to finish this PR.
Let me know which things are requried below. Then I can work more for the patch

  • test for file interface which can be written from CPython test_gzip.py
  • CPython3-only availability check for the file interface - if this is ok approach
  • Python2 and other pythons comaptibility work which including CPython _compression port

if newline is not None:
raise ValueError("Argument 'newline' not supported in binary mode")

gz_mode = mode.replace("t", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

br_mode?

@brechtm
Copy link

brechtm commented Sep 23, 2024

@youknowone This would come in very handy, so I wanted to kindly ask whether you're interested in bringing this PR to completion. Thanks!

@youknowone
Copy link
Author

@brechtm Since nobody having project ownership is interested in, I feel like spending more effort for this patch is not reasonable enough.

You can install a separated project using pip https://pypi.org/project/brotli-file/

@brechtm
Copy link

brechtm commented Sep 25, 2024

Thanks, @youknowone. I tried brotli-file, but unfortunately decompression is quite a bit slower than command-line brotli. I was planning on using brotli for compressing my CSV files which I read into pandas, but it seems pandas.read_csv() supports zstd directly (through python-zstandard), which does not suffer from a performance hit like brotli-file. Since it has better support for Python at this point, I'll be using zstd instead.

@jyrkialakuijala
Copy link
Collaborator

jyrkialakuijala commented Sep 25, 2024

@brechtm Since nobody having project ownership

There is an unreplied and unresolved review finding from eustas@ (with project ownership) here: https://github.com/google/brotli/pull/806/files/3462440035eb969771455d8812e5e7ed95e66a5f#r1058955829

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.

6 participants