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

Allow raw parameter to accept callable like body elements #233

Closed
wants to merge 1 commit into from

Conversation

ramast
Copy link
Contributor

@ramast ramast commented Jun 22, 2023

No description provided.

Copy link
Owner

@jamielennox jamielennox left a comment

Choose a reason for hiding this comment

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

I really don't remember why this wasn't available for raw - makes sense for it to be there.

@@ -70,6 +70,9 @@ Dynamic Response
================

A callback can be provided in place of any of the body elements.
raw attribute also accepts callable that returns HTTPResponse.
The HTTPResponse should have `preload_content=False` or it may not work properly.
Copy link
Owner

Choose a reason for hiding this comment

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

does it make sense to just always set raw.preload_content = False on the raw object? I forget what would happen if this isn't set?

Copy link
Owner

Choose a reason for hiding this comment

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

enforce_content_length = False should probably always be set as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we leave them as is to avoid breaking backward compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

ok, so i can't set preload_content as that is acted upon in the constructor. Setting it afterwards has no impact.

For enforce_content_length i think it should probably be false - but the whole point of using raw is that the user provides this HTTPResponse object so i guess we shouldn't do anything that changes the values they set.

Copy link
Owner

@jamielennox jamielennox left a comment

Choose a reason for hiding this comment

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

Ok, lets leave the logic of the if not raw piece. If you want to tackle it let's do it elsewhere, but i'll merge the raw CB.

@@ -70,6 +70,9 @@ Dynamic Response
================

A callback can be provided in place of any of the body elements.
raw attribute also accepts callable that returns HTTPResponse.
The HTTPResponse should have `preload_content=False` or it may not work properly.
Copy link
Owner

Choose a reason for hiding this comment

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

ok, so i can't set preload_content as that is acted upon in the constructor. Setting it afterwards has no impact.

For enforce_content_length i think it should probably be false - but the whole point of using raw is that the user provides this HTTPResponse object so i guess we shouldn't do anything that changes the values they set.

encoding = get_encoding_from_headers(headers) or 'utf-8'
content = text.encode(encoding)
if content is not None:
body = _IOReader(content)
Copy link
Owner

Choose a reason for hiding this comment

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

It's not right or wrong - but I don't want to change this block for this purpose.

The whole logic here kind of falls through to the next step based on what's been set and whilst we could reorganize it a little for efficiency it's true for all these statements, not just raw.

Unless i'm missing something please revert this bit.

@ramast
Copy link
Contributor Author

ramast commented Jan 21, 2024

I will not be able to do more work for this feature. partially because I've since moved to other projects that don't need this feature and partially because I forgot the code.

The pull request is yours, please do with it what you see fit.

@jamielennox
Copy link
Owner

I understand it's pretty old. Thanks for letting me know and I'll pick it up. Apologies again for the delay.

@ramast
Copy link
Contributor Author

ramast commented Jan 22, 2024

of course no need for apology.
Me and everyone who use your library appreciate the time and effort you put into this project

@jamielennox
Copy link
Owner

Moved to #249 as i can't push to your master branch.

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.

2 participants