-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
Changes from 13 commits
18a7eac
2e93da4
2db7e1f
beadb34
2a16f9b
f07f456
5640148
51cd8ad
c199098
58ced25
13d373f
357c8b4
b99fb9f
1cf719c
2c3a63f
c57eb8c
2174ea5
47f1656
10d4720
1a36dfc
c0e78bc
febc206
73a5a9a
1f8b27b
ac31a4a
c9e8110
61385d8
2ee88dd
d7575f0
628c2f0
5f5b2f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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`. | ||||||
epicfaace marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
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 | ||||||
|
@@ -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 | ||||||
merwok marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
.. attribute:: addinfourl.url | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>`_) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ha this is recursive! Was probably copied from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, that was a typo. Added a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
.. method:: addinfourl.geturl() | ||||||
|
||||||
Returns the URL of the resource retrieved. Recommended to use :attr:`~addinfourl.status` instead. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of:
Suggested change
|
||||||
|
||||||
10000 | .. method:: addinfourl.info() | |||||
|
||||||
Returns the response headers. Recommended to use :attr:`~addinfourl.status` instead. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should status or code be the obvious recommended attribute? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since originally both Since it seems like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that following HTTPResponse is best. Suggested doc for status property: Doc for code: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I made a typo. What I meant to say is that: So my solution was to add a property called |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. In other tests that use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The indentation here is wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
epicfaace marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
For HTTP and HTTPS URLs, this function returns a http.client.HTTPResponse | ||
object slightly modified. In addition to the three new methods above, the | ||
|
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?