-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix broken references in docs/client_reference.rst
#5620
Conversation
There are some fixes of "warnings". When I finalize PR, I will change commit messages. PR shows my progress |
I'm marking this as a draft then. |
This comment has been minimized.
This comment has been minimized.
have troubles with |
docs/client_reference.rst
Outdated
|
||
:param aiohttp.BasicAuth auth: an object that represents HTTP Basic | ||
Authorization (optional) | ||
|
||
:param version: supported HTTP version, ``HTTP 1.1`` by default. | ||
|
||
:param cookie_jar: Cookie Jar, :class:`AbstractCookieJar` instance. | ||
:param cookie_jar: Cookie Jar, :class:`aiohttp.abc.aiohttp.abc.AbstractCookieJar` instance. |
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.
This prefix is wrong as discussed in another PR.
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.
I have warning with aiohttp.abc.AbstractCookieJar
and AbstractCookieJar
. with a double prefix I don't have a warning. If you insist, I'll leave it as you want
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.
You need to fix this warning by removing the prefix from the respective .. class::
definition. It's only duplicated because it's both in the class definition and .. currentmodule::
at the top of the document — they are merged.
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.
I try it later
What do you mean? |
I did not find |
Point |
you don't understand, I did not find it in |
By the way, this reference is wrong
I don't see other places you're talking about. Maybe you forgot to update the list of warnings... |
#5620 (comment) actual version of warnings. Other places have the same situation |
Just fix the others then. |
good |
576862b
to
b1fdece
Compare
fix |
docs/client_reference.rst
Outdated
|
||
:param aiohttp.BasicAuth auth: an object that represents HTTP Basic | ||
Authorization (optional) | ||
|
||
:param version: supported HTTP version, ``HTTP 1.1`` by default. | ||
|
||
:param cookie_jar: Cookie Jar, :class:`AbstractCookieJar` instance. | ||
:param cookie_jar: Cookie Jar, :class:`aiohttp.abc.AbstractCookieJar` instance. |
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.
Let's maybe prefix this with a tilde?
:param cookie_jar: Cookie Jar, :class:`aiohttp.abc.AbstractCookieJar` instance. | |
:param cookie_jar: Cookie Jar, :class:`~aiohttp.abc.AbstractCookieJar` instance. |
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, original without tilde
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.
The original was rendered as a class name w/o a prefix
docs/client_reference.rst
Outdated
@@ -2163,7 +2163,7 @@ Connection errors | |||
|
|||
Server disconnected. | |||
|
|||
Derived from :exc:`ServerDisconnectionError` | |||
Derived from :exc:`aiohttp.ServerDisconnectionError` |
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.
Derived from :exc:`aiohttp.ServerDisconnectionError` | |
Derived from :exc:`~aiohttp.ServerDisconnectionError` |
5b7f7e7
to
5cf3b9d
Compare
Remaining warnings:
|
I think the remaining warnings go to ignore. (callable L779 not found) |
I think it's on line 111 |
Okay, I try to fix it in l111, another warnings add to ignore list and make the PR ready |
5cf3b9d
to
967f23d
Compare
Ready for review |
There's a button for that |
docs/client_reference.rst
Outdated
@@ -1213,7 +1213,7 @@ Response object | |||
|
|||
.. class:: ClientResponse | |||
|
|||
Client response returned by :meth:`ClientSession.request` and family. | |||
Client response returned by :meth:`~aiohttp.ClientSession.request` and family. |
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.
Let's preserve the original rendering
Client response returned by :meth:`~aiohttp.ClientSession.request` and family. | |
Client response returned by :meth:`ClientSession.request() <aiohttp.ClientSession.request>` and family. |
docs/client_reference.rst
Outdated
@@ -1229,7 +1229,7 @@ Response object | |||
|
|||
.. attribute:: version | |||
|
|||
Response's version, :class:`HttpVersion` instance. | |||
Response's version, :class:`aiohttp.protocol.HttpVersion` instance. |
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.
Response's version, :class:`aiohttp.protocol.HttpVersion` instance. | |
Response's version, :class:`~aiohttp.protocol.HttpVersion` instance. |
It's just a comment |
So it's not really ready for review because it's a draft, right? |
I forgot to push the button:) |
docs/conf.py
Outdated
("py:meth", "aiohttp.ClientSession.request"), # undocumented | ||
("py:class", "aiohttp.protocol.HttpVersion"), # undocumented | ||
("py:class", "aiohttp.ClientRequest"), # undocumented | ||
("py:class", "Payload"), # undocumented |
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.
This should be a full importable string, not just a class name.
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.
Do you mean full string "payload"?
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.
package_name.module_name.ClassName
, just like others.
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.
Example:
("py:class", "Payload"), # undocumented | |
("py:class", "aiohttp.payload.Payload"), # undocumented |
(
Line 135 in e8b9dbc
class Payload(ABC): |
("py:class", "Payload"), # undocumented | ||
("py:class", "aiohttp.abc.AbstractResolver"), # undocumented | ||
("py:func", "aiohttp.ws_connect"), # undocumented | ||
("py:meth", "start"), # undocumented |
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.
Same here: it should have the full context.
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.
aiohttp.start.start
?
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.
No, the thing this refers to should actually exist in the code.
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.
Hmm, It will be quite hard for me, but I try
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.
Find what it refers to in the context where it's used and that should be it
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.
Opposite :meth:(close) refers to aiohttp.WSCloseCode.OK
. Not enough context to define start
for me
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.
Alright, I did some research and it is indeed unobvious what this refers to. So let's leave it be for now.
docs/conf.py
Outdated
("py:func", "aiohttp.ws_connect"), # undocumented | ||
("py:meth", "start"), # undocumented | ||
("py:exc", "aiohttp.ServerDisconnectionError"), # undocumented | ||
("py:exc", "ClientHttpProxyError"), # undocumented |
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.
Same: it's not clear where it's defined.
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.
aiohttp. ClientHttpProxyError
?
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, check via REPL
Can you show an example, please? |
967f23d
to
098830a
Compare
098830a
to
fd2aa05
Compare
Backport to 3.8: 💚 backport PR created✅ Backport PR branch: Backported as #5734 🤖 @patchback |
Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit ff9c117)
Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit ff9c117) Co-authored-by: Olexiy Pohorely <[email protected]>
What do these changes do?
This change corrects multiple unrendered intersphinx class reference in the
client_reference.rst
document.Are there changes in behavior for the user?
NORelated issue number
#5518Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.