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

ENH Add response.text to pyodide.http.FetchResponse #4052

Merged
merged 6 commits into from Aug 28, 2023

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Aug 8, 2023

We've received feedback from users that use other requests APIs that they expect the method to be called response.text instead of response.string. Indeed both the Fetch response API and the Python requests library use this convention:

https://developer.mozilla.org/en-US/docs/Web/API/Response/text https://requests.readthedocs.io/en/latest/api/#requests.Response.text

This adds response.text to FetchResponse. It is a synonym for response.string.

This also marks response.string as deprecated but does not schedule it for removal.

cc @pelson who suggested this change.

  • Add a CHANGELOG entry
  • Add new / update outdated documentation

We've received feedback from users that use other requests APIs that
they expect the method to be called `response.text` instead of
`response.string`. Indeed both the Fetch response API and the
Python requests library use this convention:

https://developer.mozilla.org/en-US/docs/Web/API/Response/text
https://requests.readthedocs.io/en/latest/api/#requests.Response.text

This adds `response.text` to `FetchResponse`. It is a synonym for
`response.string`.
Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! Please update the changelog.

We could also deprecate response.string but I don't see any immediate need to do so.

Yes, I think we can just keep it for backward compatibility.

src/py/pyodide/http.py Show resolved Hide resolved
@rth
Copy link
Member

rth commented Aug 8, 2023

We could also deprecate response.string but I don't see any immediate need to do so.

Can we at least mention in the docstring which one should be used, and mention that the other one is deprecated in the docstring? Having two methods that do exactly the same thing is not very pythonic. But yeah I agree that there is no need to actually raise a deprecation warning.

@hoodmane
Copy link
Member Author

hoodmane commented Aug 8, 2023

Right again there are three different possible things:

  1. we tell people not to use it
  2. we tell people not to use it and warn if they do
  3. we tell people not to use it and warn if they do and have a schedule to remove it

I think I usually think "deprecated" means either 2 or 3, so far we've been using it for 3. CPython has a new "soft deprecation" which is 1. But we should make sure to make it clear what specifically we mean...

@ryanking13
Copy link
Member

  1. we tell people not to use it

I think option 1 with mentioning not to use it in the docstring is okay for this case, as the extra maintenance burden is negligible.

@ryanking13 ryanking13 added this to the 0.24.0 milestone Aug 21, 2023
@hoodmane hoodmane merged commit 303e02e into pyodide:main Aug 28, 2023
6 of 8 checks passed
@hoodmane hoodmane deleted the response.text branch August 28, 2023 11:40
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

3 participants