bpo-12707: deprecate info(), geturl(), getcode() methods in favor of headers, url, and status properties for HTTPResponse and addinfourl #11447

epicfaace · 2019-01-06T19:37:12Z

https://bugs.python.org/issue12707

…d addinfo

the-knights-who-say-ni · 2019-01-06T19:37:15Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

tirkarthi · 2019-01-06T19:56:41Z

I think there is no consensus on the issue about raising code level deprecation as noted in https://bugs.python.org/issue12707#msg234945 and this causes a lot of tests to fail to DeprecationWarning as seen in the CI. I hope it's better to split the documentation changes into a separate PR without 3.8 deprecation notice in docs and code for review.

epicfaace · 2019-01-06T22:32:07Z

@tirkarthi Makes sense, I've removed the deprecation code/notice for now. It feels odd though having multiple methods/properties that return the same thing; ideally all the duplicates should be deprecated. For now I've added the "recommended to use [x] instead" notice in the docs.

Doc/library/http.client.rst

Doc/library/urllib.request.rst

Doc/library/http.client.rst

Doc/library/urllib.request.rst

Co-Authored-By: epicfaace <aramaswamis@gmail.com>

merwok · 2019-01-07T16:35:43Z

Lib/test/test_urllib.py


    def test_getcode(self):
        self.assertIsNone(self.returned_obj.getcode())
+        self.assertIsNone(self.returned_obj.status)


I think it could be cleaner to separate these tests (recommended attributes first, then the older ones)

Ok.

In other tests that use the info(), geturl() functions, etc., do you suggest I update them to use the recommended attributes as well too?

How many changes is that?

If the functions are doc-deprecated but will never actually raise warnings, maybe it’s not worth changing tens and tens of lines.

Lib/urllib/response.py

tirkarthi · 2019-01-08T06:06:02Z

Doc/library/urllib.request.rst

+
+.. attribute:: addinfourl.status
+
+   Status code returned by server. Recommended to use :attr:`~addinfourl.status` instead.


The recommendation phrase :attr:~addinfourl.status links back to itself while building the docs locally. Also below methods geturl, info also link to addinfourl.status but it's documented to return only status code. Am I missing something here?

status seems to be a newly added property. Does this need a versionadded directive?

Ha this is recursive! Was probably copied from the getstatus docs.

Oops, that was a typo. Added a versionadded directive.

merwok · 2019-01-08T15:44:01Z

Doc/library/http.client.rst

@@ -477,6 +485,18 @@ statement.

   Is ``True`` if the stream is closed.

+.. method:: HTTPResponse.geturl()
+
+   Returns the URL of the resource retrieved. Recommended to use :attr:`~HTTPResponse.url` instead.


Maybe this could be condensed to:

Deprecated method equivalent to :attr:`HTTPResponse.url`

Is that okay to call it "deprecated" even when it doesn't return any DeprecationWarnings when used?

Yes, we name that «doc-deprecated», it’s a useful degree of deprecation.
(i.e. don’t break code that is fine, don’t annoy with warnings, just recommend the obvious way to do it in docs for people who want that)

Okay, I've went ahead and called it "deprecated" in the documentation. How do you keep track of which things mentioned in the documentation are deprecated and which are doc-deprecated?

Doc/library/urllib.request.rst

merwok · 2019-01-08T15:45:40Z

Doc/library/urllib.request.rst

+
+.. attribute:: addinfourl.headers
+
+   Returns the meta-information of the page, such as headers,


What does «such as» mean? Is there something else than HTTP response headers in the headers property?

Again, this was moved from the original documentation, but will reword.

merwok · 2019-01-08T15:46:04Z

Doc/library/urllib.request.rst

+.. attribute:: addinfourl.headers
+
+   Returns the meta-information of the page, such as headers,
+   in the form of an :func:`email.message_from_string` instance (see


See my previous comment: this should link to the EmailMessage class, not a factory function.

merwok · 2019-01-08T15:47:04Z

Doc/library/urllib.request.rst

+
+   Returns the meta-information of the page, such as headers,
+   in the form of an :func:`email.message_from_string` instance (see
+   `Quick Reference to HTTP Headers <http://jkorpela.fi/http.html>`_)


Why link to a specific personal website here? I think people know what HTTP headers are, or if they don’t other parts of the docs probably already link to general-purpose documentation (W3C or Wikipedia).

Uh, I just moved over the previous documentation that was already there. Now that you mention it, it's probably better to remove it.

Ah! In that case, keeping it is also OK. You can’t fix all the docs in one PR :)

merwok · 2019-01-08T15:47:57Z

Doc/library/urllib.request.rst

+
+.. attribute:: addinfourl.status
+
+   Status code returned by server. Recommended to use :attr:`~addinfourl.status` instead.


Suggested change

Status code returned by server. Recommended to use :attr:`~addinfourl.status` instead.

Same as *code*.

merwok · 2019-01-08T15:49:03Z

Doc/library/urllib.request.rst

+
+.. method:: addinfourl.geturl()
+
+   Returns the URL of the resource retrieved. Recommended to use :attr:`~addinfourl.status` instead.


What do you think of:

Suggested change

Returns the URL of the resource retrieved. Recommended to use :attr:`~addinfourl.status` instead.

Deprecated method equivalent to :attr:`~addinfourl.url`.

merwok · 2019-01-08T15:49:16Z

Doc/library/urllib.request.rst

+
+.. method:: addinfourl.info()
+
+   Returns the response headers. Recommended to use :attr:`~addinfourl.status` instead.


headers not status!

merwok · 2019-01-08T15:49:37Z

Doc/library/urllib.request.rst


+   Returns the status. Recommended to use :attr:`~addinfourl.status` instead.


Should status or code be the obvious recommended attribute?

Since originally both HTTPResponse and getaddrurl had the same three functions, info(), geturl(), and getcode(), and now we're replacing them with attributes instead. Both classes have .headers and .url so we're good with those two. However, there is no uniform attribute for the status code; we have HTTPResponse.status and addinfourl.code.

Since it seems like HTTPResponse.status is already documented, I thought it would be better just to rename addinfourl.code to addinfourl.status for consistency's sake.

Agreed that following HTTPResponse is best.

Suggested doc for status property: Returns the status code.

Doc for code: Same as status

Sorry I am confused: does addinfourl.code already exist in a released version? If yes, I think we should keep it, recommend status, and not add code to HTTPResponse. If no, let’s keep only getcode (old) and status (recommended, same name as other class). Sounds good?

Oops, I made a typo. What I meant to say is that: HTTPResponse.status and addinfourl.code already both exist in a released version.

So my solution was to add a property called addinfourl.status which would be recommended to use over addinfourl.code.

Lib/urllib/response.py

bedevere-bot · 2019-01-08T15:50:14Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

epicfaace · 2019-08-19T16:12:15Z

@csabella @merwok any updates on this?

matrixise

Thank you for your contribution but could you update your PR with my remarks.

Misc/NEWS.d/next/Documentation/2019-08-27-01-14-59.bpo-12707.Yj3_7_.rst

Doc/library/urllib.request.rst

matrixise · 2019-09-11T18:33:28Z

Doc/library/http.client.rst

@@ -477,6 +485,18 @@ statement.

   Is ``True`` if the stream is closed.

+.. method:: HTTPResponse.geturl()
+
+   Deprecated method equivalent to :attr:`HTTPResponse.url`.


Could you use

.. deprecated:: 3.9 The method :method:`HTTPResponse.geturl` is deprecated in favor of :attr:`HTTPResponse.url`

Check for the other attributes and the right syntax.

I disagree! Why generate a link to the section that we’re already in?

Ok, I've done something in between by starting the sentence just with "Deprecated in favor" while still keeping the deprecated:: markup.

bedevere-bot · 2019-09-11T18:33:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

epicfaace · 2019-09-13T04:26:52Z

I have made the requested changes; please review again

bedevere-bot · 2019-09-13T04:26:54Z

Thanks for making the requested changes!

@merwok, @matrixise: please review the changes made to this pull request.

matrixise

Thank you for your contribution

matrixise · 2019-09-13T10:58:41Z

@epicfaace I have fixed the .. deprecated:: 3.9 in your branch, I am waiting for the green lights from Travis and I will merge it.

I have seen an error with the deprecated directive and I can't compile the doc on my laptop

matrixise · 2019-09-13T11:41:04Z

Thank you so much for your contribution, I closed the issue and have merged this PR.

hugovk · 2023-02-05T15:32:03Z

This PR documented functions returning a status code as getstatus(), when they're really getcode().

Please could someone review PR #101296 to fix it?

epicfaace added 3 commits January 6, 2019 10:44

bpo-12707: deprecate info(), geturl(), getcode() from HTTPResponse an…

18a7eac

…d addinfo

bpo-12707: rename 'code' attribute to 'status'

2e93da4

bpo-12707: Document properties and deprecation of methods

2db7e1f

the-knights-who-say-ni added the CLA not signed label Jan 6, 2019

bedevere-bot added the awaiting review label Jan 6, 2019

epicfaace added 4 commits January 6, 2019 14:09

bpo-21257: remove deprecation warnings

beadb34

bpo-21257: fix whitespace

2a16f9b

bpo-12707: remove deprecation warnings from doc

f07f456

bpo-12707: remove deprecation warning for addurlinfo.getstatus() in docs

5640148

bpo-12707: rename code to status in test

51cd8ad

bpo-12707: rename info to headers in tests

c199098

merwok self-requested a review January 7, 2019 02:22

eamanu reviewed Jan 7, 2019

View reviewed changes

Doc/library/http.client.rst Outdated Show resolved Hide resolved

Doc/library/http.client.rst Outdated Show resolved Hide resolved

Doc/library/urllib.request.rst Outdated Show resolved Hide resolved

merwok reviewed Jan 7, 2019

View reviewed changes

Doc/library/http.client.rst Outdated Show resolved Hide resolved

merwok reviewed Jan 7, 2019

View reviewed changes

Doc/library/http.client.rst Outdated Show resolved Hide resolved

merwok reviewed Jan 7, 2019

View reviewed changes

Doc/library/urllib.request.rst Outdated Show resolved Hide resolved

eamanu and others added 3 commits January 7, 2019 08:28

Apply suggestions from code review

58ced25

Co-Authored-By: epicfaace <aramaswamis@gmail.com>

bpo-12707: Add links to doc

13d373f

bpo-12707 merge changes

357c8b4

merwok reviewed Jan 7, 2019

View reviewed changes

Lib/urllib/response.py Outdated Show resolved Hide resolved

bpo-12707: remove empty line

b99fb9f

tirkarthi reviewed Jan 8, 2019

View reviewed changes

merwok requested changes Jan 8, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jan 8, 2019

csabella reopened this May 22, 2019

📜🤖 Added by blurb_it.

2ee88dd

merwok approved these changes Aug 27, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 27, 2019

matrixise requested changes Sep 11, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 11, 2019

epicfaace added 2 commits September 13, 2019 04:24

Make NEWS more clear

d7575f0

add Deprecated warnings and update versionadded

628c2f0

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 13, 2019

bedevere-bot requested review from matrixise and merwok September 13, 2019 04:26

matrixise previously approved these changes Sep 13, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 13, 2019

Fix the call of the 'deprecated' directive

5f5b2f9

matrixise merged commit ff2e182 into python:master Sep 13, 2019

bedevere-bot removed the awaiting merge label Sep 13, 2019

hugovk mentioned this pull request Jan 24, 2023

Docs: Fix getstatus() -> getcode() typos #101296

Merged

dimo414 mentioned this pull request Jan 9, 2024

Usage of HTTPResponse.url #86228

Open


		.. attribute:: addinfourl.status

		Status code returned by server. Recommended to use :attr:`~addinfourl.status` instead.


		.. attribute:: addinfourl.headers

		Returns the meta-information of the page, such as headers,

	Status code returned by server. Recommended to use :attr:`~addinfourl.status` instead.
	Same as code.


		.. method:: addinfourl.geturl()

		Returns the URL of the resource retrieved. Recommended to use :attr:`~addinfourl.status` instead.

	Returns the URL of the resource retrieved. Recommended to use :attr:`~addinfourl.status` instead.
	Deprecated method equivalent to :attr:`~addinfourl.url`.


		.. method:: addinfourl.info()

		Returns the response headers. Recommended to use :attr:`~addinfourl.status` instead.


		Returns the status. Recommended to use :attr:`~addinfourl.status` instead.

bpo-12707: deprecate info(), geturl(), getcode() methods in favor of headers, url, and status properties for HTTPResponse and addinfourl #11447

bpo-12707: deprecate info(), geturl(), getcode() methods in favor of headers, url, and status properties for HTTPResponse and addinfourl #11447

Conversation

epicfaace commented Jan 6, 2019 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Jan 6, 2019

tirkarthi commented Jan 6, 2019

epicfaace commented Jan 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epicfaace Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 8, 2019

epicfaace commented Aug 19, 2019

matrixise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 11, 2019

epicfaace commented Sep 13, 2019

bedevere-bot commented Sep 13, 2019

matrixise left a comment

Choose a reason for hiding this comment

matrixise commented Sep 13, 2019

matrixise commented Sep 13, 2019

hugovk commented Feb 5, 2023

epicfaace commented Jan 6, 2019 •

edited by bedevere-bot

Loading

epicfaace Jan 9, 2019 •

edited

Loading