python-hyper / brotlicffi

Python bindings to the Brotli compression library
MIT License
147 stars 27 forks source link

Add process() for compatibility, example project for using C and CFFI bindings #168

Closed sethmlarson closed 3 years ago

sethmlarson commented 3 years ago

Would like a review on the recommendation to use brotlicffi on implementation_name != 'cpython' and to use Brotli on CPython. This links us at the hip to the API of the C bindings and will vastly decrease how often we're used, but I think this is a desirable for our users.

Kriechi commented 3 years ago

I've only loosely followed the discussion around this topic - so please feel free to disregard my proposal if it doesn't make any sense at all:

Instead of burdening the user with doing the requirements dance of defining the right dependency based on the implementation name / runtime, how about pushing it one layer down? We could provide a dummy package under the current name, and it would require the correct sub-package by itself. Our users then don't have to worry about doing the right steps from the example projects.

sethmlarson commented 3 years ago

@Kriechi Good question!

So I did an investigation on how packages require Brotli/Brotlipy currently we can see that a good portion are doing exactly what we're recommending here and some are only requiring Brotlipy. I'm thinking we lean into this pattern by making an official recommendation and by making using this pattern simpler. It's less for both us and our downstreams to maintain.

After this release I'm planning on making PRs to projects that aren't following the pattern to align them with our recommendation.

As far as I can tell this is the most common way people end up with a brotli library on their machine as Brotli vs brotlipy downloads are heavily in favor of Brotli. It's unlikely they install directly (unless typing pip install brotli which for the most part ends up with a fine solution)

sethmlarson commented 3 years ago

@Kriechi Let me know if the above sounds good to you, I'll hold off on merging until then