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

Added param response_process (callback) to process response object #10

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sohang3112
Copy link

Requests to some urls return binary data which cannot be decoded (eg. image data).
In these cases, param parse_bytes can be set to False.

@sohang3112
Copy link
Author

@singhsidhukuldeep Please review my pull request, and let me know if something can be improved. Thanks in advance 🙂

@singhsidhukuldeep
Copy link
Owner

Please resolve conflicts

@sohang3112
Copy link
Author

@singhsidhukuldeep Resolved the conflicts - they were because of the commits in main branch since February.

@sohang3112
Copy link
Author

@singhsidhukuldeep Did you have a chance to review the PR yet?

@singhsidhukuldeep
Copy link
Owner

I don't think there is any need for this, you can set parse_json as False

This will give you the raw response and later you can process it as you want

@sohang3112
Copy link
Author

you can set parse_json as False

@singhsidhukuldeep Setting parse_json to False does not help, because the request bytes are always decoded irrespective of parse_json. In the following code from __init__.py:

data = response.read()
encoding = response.info().get_content_charset("utf-8")
decoded_data = data.decode(encoding)                      # THE BYTES ARE ALWAYS DECODED TO STRING FIRST
self.results[loc] = (
    json.loads(
        decoded_data) if self.parse_json else decoded_data
)
self.queue.task_done()

Decoding the bytes is always done (data.decode(encoding)). This is not desirable for binary files like images, videos, etc.

@singhsidhukuldeep
Copy link
Owner

Makes sense, let's make it parse JSON or parse nothing and return the Request object itself

@sohang3112
Copy link
Author

Makes sense, let's make it parse JSON or parse nothing and return the Request object itself

That sounds good. But this will break compatibility for existing users - when parse_json=False, they would expect the response text, but would instead get the response object.

@singhsidhukuldeep
Copy link
Owner

@sohang3112 need to rethink this; people will start adding specific post processes for their own use cases

We need to keep it light-weight and extendable

one idea is to allow the user to pass their own post-processing logic as a callable function (post_process_func), We will pass the request object through that, this can overwrite parse_json when not None

@sohang3112
Copy link
Author

sohang3112 commented Sep 3, 2023

@singhsidhukuldeep Yes this sounds like a good idea - if a callable is passed, then pass it the request object and use its result. Otherwise do existing processing.

@sohang3112 sohang3112 closed this Sep 3, 2023
@sohang3112 sohang3112 reopened this Sep 3, 2023
@sohang3112
Copy link
Author

Sorry closed accidentally 😞
Re-opening..

@sohang3112
Copy link
Author

if a callable is passed, then pass it the request object and use its result. Otherwise do existing processing.

@singhsidhukuldeep Should I modify my PR to do this??

@singhsidhukuldeep
Copy link
Owner

singhsidhukuldeep commented Sep 12, 2023 via email

@sohang3112 sohang3112 changed the title Added param parse_json to avoid parsing binary data (eg. image bytes) Added param response_process (callback) to process response object Sep 13, 2023
@sohang3112 sohang3112 changed the title Added param response_process (callback) to process response object Added param response_process (callback) to process response object Sep 13, 2023
Old param (parse_bytes) was removed as it's no longer used in code
@sohang3112
Copy link
Author

sohang3112 commented Sep 13, 2023

@singhsidhukuldeep I updated the PR to accept optional callback response_process which is used like this:

response_ok, data = response_process(response)

data is used if response_ok is true, otherwise request will be retried.

@sohang3112
Copy link
Author

@singhsidhukuldeep Please review and merge my PR. Let me know if any further changes are needed.

@sohang3112
Copy link
Author

@singhsidhukuldeep Please merge my PR. Let me know if any further changes are required from my side.

@sohang3112
Copy link
Author

@singhsidhukuldeep Did you have a chance to review the PR yet? As I mentioned previously, I had modified the PR to include the changes we discussed.

@sohang3112
Copy link
Author

@singhsidhukuldeep Hi. Did you have a look at the PR yet? As I mentioned before, I modified the PR to include the changes we discussed.

@singhsidhukuldeep
Copy link
Owner

Hi @sohang3112
I am occupied with few things, will close this shortly

@sohang3112
Copy link
Author

@singhsidhukuldeep Please review & merge this PR now.

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

2 participants