Skip to content
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

Merged
merged 6 commits into from
Feb 23, 2024
Merged

Fix broken links in docs #6583

merged 6 commits into from
Feb 23, 2024

Conversation

EFord36
Copy link
Contributor

@EFord36 EFord36 commented Nov 24, 2023

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.

@@ -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!
Copy link
Contributor Author

@EFord36 EFord36 Nov 24, 2023

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.

@@ -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>`_
Copy link
Contributor

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

Copy link
Contributor Author

@EFord36 EFord36 Nov 24, 2023

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>`_)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@EFord36
Copy link
Contributor Author

EFord36 commented Nov 28, 2023

@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!

Copy link

@Jamim Jamim left a 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>`_.
Copy link

@Jamim Jamim Jan 25, 2024

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.

Copy link
Contributor Author

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.

@EFord36
Copy link
Contributor Author

EFord36 commented Jan 26, 2024

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 build checks for any python >3.7 are failing with this error:

ImportError: cannot import name 'parse_authorization_header' from 'werkzeug.http' (/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/site-packages/werkzeug/http.py)

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 wekzeug dependency-related issue to me.

@Jamim
Copy link

Jamim commented Jan 27, 2024

I'm guessing you don't have write access

That's true. I just wanted to support your contribution 😃

@nateprewitt nateprewitt added this to the 2.32.0 milestone Feb 21, 2024
@sigmavirus24
Copy link
Contributor

@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.
@EFord36
Copy link
Contributor Author

EFord36 commented Feb 23, 2024

@EFord36 would you be willing to rebase/resolve the conflicts here?

Hi,

I've done that now, thanks for the reminder!

@sigmavirus24
Copy link
Contributor

Thank you!

@sigmavirus24 sigmavirus24 merged commit f3f2611 into psf:main Feb 23, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants