Warn on all redirects if linkcheck_allowed_redirects is an empty dictionary

[AA-Turner]

Purpose

See discussion in #13439, cc @jameslamb.

References

• linkcheck only emits warnings on HTTP redirects if linkcheck_allowed_redirects is non-empty #13439

[jayaddison]

Missing an explanatory message here?

[jayaddison]

Perhaps we should update the default linkcheck_allowed_redirects value in the documentation to None? (since it's not really a default of an empty-dict anymore)

[jayaddison]

This line/diff is confusing without context -- but the way I understand this: this code is within the _Status.REDIRECTED case -- and we can only reach this part of the code if a disallowed redirect occurred, or if no filtering of allowed redirects is configured. So the conditional check here is a shortcut, simply confirming whether allowed redirects are configured or not; if it is configured, we should emit a warning.

[jayaddison]

Generally looks good to me. I'll open a pull request with the two nitpicky suggestions I had (an update default in the config docs, and an exception message).

[pllim]

Hi. Is it possible that this PR introduced a regression? We see a new failure at astropy/sphinx-astropy#80 for sphinx-dev but we have no changed anything related to linkcheck lately.

The only specific linkcheck setting we touch is this at https://github.com./astropy/sphinx-astropy/blob/9ee5275a8ccc09f434dc54f66993dbd0356d7200/sphinx_astropy/conf/v2.py#L386

linkcheck_timeout = 60

So the warning below is unexpected unless I missed something?

WARNING: The config value `linkcheck_allowed_redirects' has type `NoneType'; expected `dict'.

Hope you can advise. Thanks!

[jayaddison]

Thanks @pllim for reporting this - that does seem likely to be related to this pull request, yep. I would have expected the config to use the default sentinel value for linkcheck_allowed_redirects added in this PR; for some reason it appears that was not the case. Would you mind opening an issue report for this?

[pllim]

Thank you for your reply. I opened #13462

[jayaddison]

Thanks @pllim - I'll aim to investigate that within the next day or two.

[jayaddison]

Note: two of our existing linkcheck test cases pass somewhat-by-quirk, following this PR; they are:

• tests/test_builders/test_build_linkcheck.py::test_follows_redirects_on_HEAD
• tests/test_builders/test_build_linkcheck.py::test_follows_redirects_on_GET

Patching them so that compile_linkcheck_allowed_redirects is invoked -- as it is when a genuine sphinx-build takes place, results in them failing.

--- a/tests/test_builders/test_build_linkcheck.py
+++ b/tests/test_builders/test_build_linkcheck.py
@@ -712,6 +712,7 @@ def log_date_time_string(self):
 )
 def test_follows_redirects_on_HEAD(app, capsys):
     with serve_application(app, make_redirect_handler(support_head=True)) as address:
+        compile_linkcheck_allowed_redirects(app, app.config)
         app.build()
     _stdout, stderr = capsys.readouterr()
     content = (app.outdir / 'output.txt').read_text(encoding='utf8')
@@ -735,6 +736,7 @@ def test_follows_redirects_on_HEAD(app, capsys):
 )
 def test_follows_redirects_on_GET(app, capsys):
     with serve_application(app, make_redirect_handler(support_head=False)) as address:
+        compile_linkcheck_allowed_redirects(app, app.config)
         app.build()
     _stdout, stderr = capsys.readouterr()
     content = (app.outdir / 'output.txt').read_text(encoding='utf8')

That's a rough edge in the existing linkcheck builder testsuite -- ideally we'd not require test authors to include that step themselves (it can easily be omitted).