Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Close #6525: linkcheck: Add linkcheck_ignore_redirects and linkcheck_warn_redirects #9234
Close #6525: linkcheck: Add linkcheck_ignore_redirects and linkcheck_warn_redirects #9234
Changes from 5 commits
05eb2ca
707319a
ce9e2e6
c97e9b1
4201a84
fe89a07
676834b
48c80b1
2887dd0
988a79d
5e5bca9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If there's no regex capture then this isn't checking what it implies it's checking. To allow for a more complete example without comparing the source and destination paths, I've added an allowance for redirecting to
www.
but this could be removed too.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 think the example should use patterns for both source and canonical URIs. So I think the current one is enough to describe the feature.
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.
But this is not just allowing redirects to the canonical version of a URL, it's allowing any redirects.
You may as well use
r'https://sphinx-doc\.org/.*': r'https://sphinx-doc\.org/.*'
because there isn't really any validation of the destination.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.
What do you mean? The current rule verifies the pages under
https://sphinx-doc.org/
to the URL underhttps://sphinx-doc.org/en/master/
. It does not mean "any redirects".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 allowing practically any redirect, so not what I would consider a good example.
It's not far off allowing
https://www\.example\.com/.*
tohttps://www\.example\.com/.*
which then causes linkcheck to ignore removed pages that just redirect to the home page.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.
If
https://sphinx-doc.org/foo/bar
redirects tohttps://sphinx-doc.org/other/path
, it's not "allowed". Is this allowing any redirect? I'm confused.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 specification from the issue also asks for a mean to reuse match groups from the
from_url
in theto_url
.IIUC, the issue asks something similar to:
I am unsure if that is feasible at all.
From the
sub()
documentation (used byexpand()
):There’s always the option of documenting that the canonical URI cannot contain special sequences (e.g.
\d
,\w
, etc.), but that’s sure to bite most people.Another idea to tighten the validation would be to use named groups in both the source URI and canonical URI, and compare the named groups. Something like:
That may not be as robust as one might expect. For example, I can imagine somebody documenting their regexp with capturing groups. IDK if the extra validation is worth the effort?
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.
If
https://www.sphinx-doc.org/en/stable/usage/quickstart.html
redirects to something other thanhttps://www.sphinx-doc.org/en/master/usage/quickstart.html
then it implies that the page no longer exists in the stable version and it should be possible to consider that an error. The validation should be worth the effort if someone is taking the effort to link to/stable/
versions (although I'm not linking to readthedocs websites like this myself).I would be inclined to use
re.sub()
and document it. Alternatively, do the group dict checking but only if the group name exists in both matches?There may also need to be a regex and non-regex version because the need to escape every
.
for an accurate match may confuse people. Any examples should definitely user'...'
strings.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.
For a page that does not exist, the server should return a 404 in the first place.
I understand not all web servers behave that way, but let’s keep in mind we’re dealing with poorly configured web servers here. I wouldn’t advise implementing a brittle solution to work around bad behaving servers. I consider the
re.sub()
solution to be brittle:\
in theto_url
is sure to trip a good portion of users,re.sub()
was design to perform replacements in a template string, not in a regular expression. There’s no guarantee the regular expression will be valid after the templating. The\
is a symptom of it, but there may be others.The comparison of groups seems more robust to me. One can adjust the groups to capture the same portion on both sides. It needs some work to experiment (e.g. try to implement the validation in the #6525 with that system). I’ll try it in the days to come.
I think it would mostly be confusing. There would be two ways of doing the same thing. Besides, there’s a strong probability the unexpected redirect URL will be different enough that the regexp won’t match, even if
r"."
is used insteadr"\."
. Not discussing thatr"\."
is tighter and correct, simply trying to explain why a second setting without regexp seems overkill to 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.
As @nomis says, the idea here is that it is possible to accept redirect targets matching a certain pattern to be considered "working" and, if they don't match, then they should still show up as "redirected".
I've had a play with the example that @francoisfreitag posted and think that we can make this work by escaping non-backreference escapes:
So
re.sub(r'(\\[^0-9g])', r'\\\1', to_url)
seems to do the trick, escaping non-backreference escapes. It seems that.expand()
also reverses this extra escaping for us.