8000 bpo-12707: deprecate info(), geturl(), getcode() methods in favor of headers, url, and status properties for HTTPResponse and addinfourl by epicfaace · Pull Request #11447 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

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

Merged
merged 31 commits into from
Sep 13, 2019
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
18a7eac
bpo-12707: deprecate info(), geturl(), getcode() from HTTPResponse an…
epicfaace Jan 6, 2019
2e93da4
bpo-12707: rename 'code' attribute to 'status'
epicfaace Jan 6, 2019
2db7e1f
bpo-12707: Document properties and deprecation of methods
epicfaace Jan 6, 2019
beadb34
bpo-21257: remove deprecation warnings
epicfaace Jan 6, 2019
2a16f9b
bpo-21257: fix whitespace
epicfaace Jan 6, 2019
f07f456
bpo-12707: remove deprecation warnings from doc
epicfaace Jan 6, 2019
5640148
bpo-12707: remove deprecation warning for addurlinfo.getstatus() in docs
epicfaace Jan 6, 2019
51cd8ad
bpo-12707: rename code to status in test
epicfaace Jan 7, 2019
c199098
bpo-12707: rename info to headers in tests
epicfaace Jan 7, 2019
58ced25
Apply suggestions from code review
eamanu Jan 7, 2019
13d373f
bpo-12707: Add links to doc
epicfaace Jan 7, 2019
357c8b4
bpo-12707 merge changes
epicfaace Jan 7, 2019
b99fb9f
bpo-12707: remove empty line
epicfaace Jan 7, 2019
1cf719c
bpo-12707: fix typos
epicfaace Jan 9, 2019
2c3a63f
bpo-12707: add versionadded directive
epicfaace Jan 9, 2019
c57eb8c
bpo-12707: recommendation to use addinfourl.status instead of .code
epicfaace Jan 9, 2019
2174ea5
Update Doc/library/urllib.request.rst
merwok Jan 9, 2019
47f1656
bpo-12707: use email.message.EmailMessage
epicfaace Jan 9, 2019
10d4720
bpo-12707: clean up wording for headers
epicfaace Jan 9, 2019
1a36dfc
Merge branch 'bpo-12707' of https://github.com/epicfaace/cpython into…
epicfaace Jan 9, 2019
c0e78bc
bpo-12707: re-add deprecation wording in doc
epicfaace Jan 11, 2019
febc206
bpo-12707: Separate tests for new and old attributes
epicfaace Jan 11, 2019
73a5a9a
Update Lib/urllib/request.py
merwok Jan 15, 2019
1f8b27b
bpo-12707: fix indentation and formatting
epicfaace Jan 15, 2019
ac31a4a
Merge branch 'bpo-12707' of https://github.com/epicfaace/cpython into…
epicfaace Jan 15, 2019
c9e8110
bpo-12707: fix indentation
epicfaace Jan 15, 2019
61385d8
bpo-12707: remove reST from docstring
epicfaace Jan 15, 2019
2ee88dd
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 27, 2019
d7575f0
Make NEWS more clear
epicfaace Sep 13, 2019
628c2f0
add Deprecated warnings and update versionadded
epicfaace Sep 13, 2019
5f5b2f9
Fix the call of the 'deprecated' directive
matrixise Sep 13, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
20 changes: 20 additions & 0 deletions Doc/library/http.client.rst
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,14 @@ statement.

HTTP protocol version used by server. 10 for HTTP/1.0, 11 for HTTP/1.1.

.. attribute:: HTTPResponse.url

URL of the resource retrieved, commonly used to determine if a redirect was followed.

.. attribute:: HTTPResponse.headers

Headers of the response in the form of an :meth:`email.message_from_string` instance.

.. attribute:: HTTPResponse.status

Status code returned by server.
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be condensed to:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


.. method:: HTTPResponse.info()

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

.. method:: HTTPResponse.getstatus()

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

Examples
--------

Expand Down
51 changes: 36 additions & 15 deletions Doc/library/urllib.request.rst
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,8 @@ The :mod:`urllib.request` module defines the following functions:
The *cadefault* parameter is ignored.

This function always returns an object which can work as a
:term:`context manager` and has methods such as

* :meth:`~urllib.response.addinfourl.geturl` --- return the URL of the resource retrieved,
commonly used to determine if a redirect was followed

* :meth:`~urllib.response.addinfourl.info` --- return 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>`_)

* :meth:`~urllib.response.addinfourl.getcode` -- return the HTTP status code of the response.
:term:`context manager` and has the properties `url`, `headers`, and `status`.
See :class:`urllib.response.addinfourl` for more detail on these properties.

For HTTP and HTTPS URLs, this function returns a
:class:`http.client.HTTPResponse` object slightly modified. In addition
Expand Down Expand Up @@ -1557,9 +1549,38 @@ some point in the future.
:synopsis: Response classes used by urllib.

The :mod:`urllib.response` module defines functions and classes which define a
minimal file like interface, including ``read()`` and ``readline()``. The
typical response object is an addinfourl instance, which defines an ``info()``
method and that returns headers and a ``geturl()`` method that returns the url.
Functions defined by this module are used internally by the
:mod:`urllib.request` module.
minimal file-like interface, including ``read()`` and ``readline()``.
Functions defined by this module are used internally by the :mod:`urllib.request` module.
The typical response object is a :class:`urllib.response.addinfourl` instance:

.. class:: urllib.response.addinfourl

.. attribute:: addinfourl.url
Copy link
Member

Choose a reason for hiding this comment

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

To avoid repeating the class name, nest the attribute/method directives under the class directive:

.. class:: addinfourl

   .. attribute:: headers

(adding the module name to the class is not needed because the file has a module directive at the top)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks.


URL of the resource retrieved, commonly used to determine if a redirect was followed.

.. attribute:: addinfourl.headers

Returns the meta-information of the page, such as headers,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

in the form of an :func:`email.message_from_string` instance (see
Copy link
Member

Choose a reason for hiding this comment

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

See my previous comment: th 9E7A is should link to the EmailMessage class, not a factory function.

`Quick Reference to HTTP Headers <http://jkorpela.fi/http.html>`_)
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


.. attribute:: addinfourl.code

Status code returned by server.

.. attribute:: addinfourl.status

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

Choose a reason for hiding this comment

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

  • 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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Member

Choose a reason for hiding this comment

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

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`.


10000 .. method:: addinfourl.info()

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

Choose a reason for hiding this comment

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

headers not status!


.. method:: addinfourl.getstatus()

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

Choose a reason for hiding this comment

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

Should status or code be the obvious recommended attribute?

Copy link
Contributor Author
@epicfaace epicfaace Jan 9, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that following HTTPResponse is best.

Suggested doc for status property: Returns the status code.

Doc for code: Same as status

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

3 changes: 3 additions & 0 deletions Lib/test/test_urllib.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,12 +187,15 @@ def test_close(self):

def test_info(self):
self.assertIsInstance(self.returned_obj.info(), email.message.Message)
self.assertIsInstance(self.returned_obj.headers, email.message.Message)

def test_geturl(self):
self.assertEqual(self.returned_obj.geturl(), self.pathname)
self.assertEqual(self.returned_obj.url, self.pathname)

def test_getcode(self):
self.assertIsNone(self.returned_obj.getcode())
self.assertIsNone(self.returned_obj.status)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.


def test_iter(self):
# Test iterator
Expand Down
4 changes: 4 additions & 0 deletions Lib/test/test_urllib_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def closehook():
def test_addinfo(self):
info = urllib.response.addinfo(self.fp, self.test_headers)
self.assertEqual(info.info(), self.test_headers)
self.assertEqual(info.headers, self.test_headers)

def test_addinfourl(self):
url = "http://www.python.org"
Expand All @@ -51,6 +52,9 @@ def test_addinfourl(self):
self.assertEqual(infourl.info(), self.test_headers)
self.assertEqual(infourl.geturl(), url)
self.assertEqual(infourl.getcode(), code)
self.assertEqual(infourl.headers, self.test_headers)
self.assertEqual(infourl.url, url)
self.assertEqual(infourl.status, code)

def tearDown(self):
self.sock.close()
Expand Down
14 changes: 3 additions & 11 deletions Lib/urllib/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,18 +163,10 @@ def urlopen(url, data=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,

The *cadefault* parameter is ignored.

This function always returns an object which can work as a context
manager and has methods such as

* geturl() - return the URL of the resource retrieved, commonly used to
determine if a redirect was followed

* info() - return the meta-information of the page, such as headers, in the
form of an email.message_from_string() instance (see Quick Reference to
HTTP Headers)

* getcode() - return the HTTP status code of the response. Raises URLError
on errors.
This function always returns an object which can work as a
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation here is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, thanks!

:term:`context manager` and has the properties `url`, `headers`, and `status`.
See `urllib.response.addinfourl` for more detail on these properties.

For HTTP and HTTPS URLs, this function returns a http.client.HTTPResponse
object slightly modified. In addition to the three new methods above, the
Expand Down
4 changes: 4 additions & 0 deletions Lib/urllib/response.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ def __init__(self, fp, headers, url, code=None):
self.url = url
self.code = code

@property
def status(self):
return self.code

def getcode(self):
return self.code

Expand Down
0