-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 links in docs #6583
Fix broken links in docs #6583
Conversation
docs/community/out-there.rst
Outdated
@@ -4,7 +4,7 @@ Integrations | |||
Python for iOS | |||
-------------- | |||
|
|||
Requests is built into the wonderful `Python for iOS <https://itunes.apple.com/us/app/python-2.7-for-ios/id485729872?mt=Python8>`_ runtime! | |||
Requests is built into the wonderful `Python for iOS <https://apps.apple.com/us/app/python-2-5-for-ios/id577916777>`_ runtime! |
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 believe this is a similar app by the same developer, but it's python 2.5 (long deprecated), and doesn't work on the most recent version of iOS anyway.
I was tempted to just delete the whole 'Python for iOS' section here, but I thought I would see what a maintainer thought before deleting the whole section.
docs/community/out-there.rst
Outdated
@@ -15,7 +15,6 @@ Articles & Talks | |||
================ | |||
- `Python for the Web <https://www.gun.io/blog/python-for-the-web>`_ teaches how to use Python to interact with the web, using Requests. | |||
- `Daniel Greenfeld's Review of Requests <https://pydanny.blogspot.com/2011/05/python-http-requests-for-humans.html>`_ | |||
- `Issac Kelly's 'Consuming Web APIs' talk <https://issackelly.github.com/Consuming-Web-APIs-with-Python-Talk/slides/slides.html>`_ |
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 is actually an easy one to fix: https://issackelly.github.com/Consuming-Web-APIs-with-Python-Talk/slides/slides.html is from the very old GitHub pages and they switched the domain from github.com
to github.io
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.
ah, can't believe I didn't think of that 🤦 Thanks for finding it!
I've updated the commit to 'fix' it to the correct link rather than removing the line.
AUTHORS.rst
Outdated
@@ -124,7 +124,7 @@ Patches and Suggestions | |||
- Bryce Boe <bbzbryce@gmail.com> (`@bboe <https://github.com/bboe>`_) | |||
- Colin Dunklau <colin.dunklau@gmail.com> (`@cdunklau <https://github.com/cdunklau>`_) | |||
- Bob Carroll <bob.carroll@alum.rit.edu> (`@rcarz <https://github.com/rcarz>`_) | |||
- Hugo Osvaldo Barrera <hugo@barrera.io> (`@hobarrera <https://github.com/hobarrera>`_) |
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'd rather not remove these. I don't think there's value in removing these links to folks even if they've deleted their account.
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.
Fair enough - I've removed all cases like this from the relevant commit.
I've left in the case where the GitHub user has changed, but let me know if you want me to remove that too.
@sigmavirus24 Thanks for your comments - I think this is ready to look at again whenever you have time. No rush from my side, I'm sure you're busy, just didn't want it to get stuck in process if you thought I was still working on 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.
That's great! Thank you, @EFord36!
I believe these changes are ready to be merged.
@@ -1097,7 +1097,7 @@ The **connect** timeout is the number of seconds Requests will wait for your | |||
client to establish a connection to a remote machine (corresponding to the | |||
`connect()`_) call on the socket. It's a good practice to set connect timeouts | |||
to slightly larger than a multiple of 3, which is the default `TCP packet | |||
retransmission window <https://www.hjp.at/doc/rfc/rfc2988.txt>`_. | |||
retransmission window <https://datatracker.ietf.org/doc/html/rfc2988>`_. |
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.
It's a good practice to set connect timeouts to slightly larger than a multiple of 3, which is the default TCP packet retransmission window.
Despite the fact I already approved this PR, there's still a room for improvement.
RFC 2988 was obsoleted by RFC 6298, so this link might be updated even further, but it would require additional changes since newer RFC makes the whole statement not correct due to this change.
So this might be handled in an additional 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.
Thanks - seems like a sensible comment, agree it makes sense for an additional PR.
Thanks @Jamim for the review! It looks like GitHub still wants an approval from a reviewer with write access - I'm guessing you don't have write access (just checking in case something has gone wrong with the process)? I've just rebased against main so this is fresh and to give the GitHub checks another change to pass - looks like the
But the 3.7 ones pass, and this error also occurred in the checks for the latest commit to main, so this seems to be an unrelated |
That's true. I just wanted to support your contribution 😃 |
@EFord36 would you be willing to rebase/resolve the conflicts here? |
Update account link for original author which has changed.
The rendered docs 'auto-link' this, which might encourage users to click the link, even though it doesn't go anywhere.
ietf seems appropriate here - it's used elsewhere in the requets docs in several places.
Hi, I've done that now, thanks for the reminder! |
Thank you! |
Hi,
I noticed a broken link to rfc2988 in the timeout section of the docs and thought I would fix it.
While doing that, I thought I would run the sphinx
linkcheck
builder to check for other broken links and fix them.There were a few, some where I've 'fixed'/updated the link are hopefully uncontroversial.
Some are more controversial - removing sections written by the original author of requests that now have dead links, removing talks with dead links to slides, removing people's now dead GitHub account links. I wasn't sure what the right thing to do here was, but I went with my intuition, and kept commits pretty discrete so I can drop/amend them if the maintainers want me to do something different. If some are controversial enough to slow down the PR, I can always pull out the uncontroversial ones into their own PR for faster merging.