search

daggaz / json-stream

Simple streaming JSON parser and encoder.
MIT License
122 stars 18 forks source link

Add `httpx` support #28

Closed vtbassmatt closed 1 year ago

vtbassmatt commented 1 year ago

Fixes https://github.com/daggaz/json-stream/issues/27

vtbassmatt commented 1 year ago

I tried to make each commit reviewable on its own, though the total PR is relatively readable, too.

The duplication between the httpx and requests modules (and their tests) feels unfortunate. If you're open to renaming the requests module something more generic, then we could do something like getattr(response, 'iter_content') vs getattr(response, 'iter_bytes') to see which kind of Response we're dealing with. But I don't feel at all strongly about the topic.

daggaz commented 1 year ago

I agree that the duplication between the modules is unfortunate.

In theory you could factor out the "thing that gives you an interable" (specific to requests/httpx) from the "thing that processes an iterable" (maybe change the interface to json_stream.load() to accept a file or an iterable and wrap appropriately.

But I'm happy to accept this PR as is (pending the resolution of my comments) and I can roll up the refactoring work into some bigger plans I have (related to async support).

vtbassmatt commented 1 year ago

🙇 thanks for the review, and for being open to including these tweaks! I think I got everything.