Skip to content

Doc implement reredirects #19456

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 25, 2021
Merged

Doc implement reredirects #19456

merged 4 commits into from
Feb 25, 2021

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Feb 4, 2021 •
edited
Loading

PR Summary

UPDATE:

This is based off #19440 just because that is a good example where some existing files were deleted.

This now uses a vendored version https://github.com/anntzer/sphinx-redirectfrom @anntzer put together. In #19440 doc/api/backend_qt4agg_api.rst was removed, and the info is now in doc/api/backend_qt_api.rst so doc/api/backend_qt4agg_api.rst now has a redirect-from directive:

.. redirect-from:: /api/backend_qt4agg_api

When the build is made this makes build/html/backend_qt4agg_api.html which has a refresh:

<html>
  <head>
    <meta http-equiv="refresh" content="0; url=backend_qt_api.html">
    <link rel="canonical" href="backend_qt_api.html">
  </head>
</html>

so the old link still exists.

(probably not super necessary for this example, but in general should be quite useful).

I think there are in general some major disadvantages compared with the reredirect approach. Namely that moving whole directories will require each file in the directory getting this directive to its new location. OTOH, that is pretty easy to script up if someone needs to do a bulk move.

  • add instructions in documentation guide about how to do this

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@jklymak jklymak force-pushed the doc-impliment-reredirects branch 3 times, most recently from 74331a4 to abeef9d Compare February 5, 2021 05:01
@QuLogic
Copy link
Member

QuLogic commented Feb 5, 2021

The placeholders cause the build to fail:

checking consistency... /home/circleci/project/doc/api/backend_gtk3agg_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_gtk3cairo_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_qt4agg_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_qt4cairo_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_qt5agg_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_qt5cairo_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_tkagg_api.rst: WARNING: document isn't included in any toctree
/home/circleci/project/doc/api/backend_wxagg_api.rst: WARNING: document isn't included in any toctree
done

@jklymak jklymak marked this pull request as draft February 5, 2021 11:49
@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2021

The placeholders cause the build to fail:

Yep. Those can be orphaned. Looks like the links need to be relative to the directory that they are being made in as well (despite what the docs say). Making them absolute to root probably works too, but doesn't work in the artifacts html...

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2021

Well, that works. I'm not a huge fan to be honest, and almost wish we could just have a new sphinx directive. Is that easy? :redirect: ./newpage would be a lot easier than orphaning and adding to the conf.py.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2021

To see how this work, navigate to https://53094-1385122-gh.circle-artifacts.com/0/doc/build/html/api/backend_gtk3agg_api.html and it will take you to
https://53094-1385122-gh.circle-artifacts.com/0/doc/build/html/api/backend_gtk3_api.html

@anntzer
Copy link
Contributor

anntzer commented Feb 5, 2021

I'll have a look at the directive possibility.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2021

Cool, it should be pretty straightforward. I may look at it as well, but probably a bit of learning curve. For reference, all the reredirects does is change the html to:

<html><head><meta http-equiv="refresh" content="0; url=${to_uri}"></head></html>

It might be nice to spice that up a bit (add a canonical?) but I think even as-is search engines will recognize that this is not supposed to be indexed.

@anntzer
Copy link
Contributor

anntzer commented Feb 5, 2021

@jklymak See https://github.com/anntzer/sphinx-redirectfrom (quick hack, barely tested, unlikely to eat your dog though). I went for what I think is a slightly nicer approach of not actually requiring to keep the old files in the source tree and having a .. redirect-from:: /path/to/old/src in the new source tree.

@jklymak
Copy link
Member Author

jklymak commented Feb 5, 2021

Nice, I think I agree thats better, because if we delete the new target, it would be nice to know we have to redirect again. I'll play with that over the w/e. Thanks a lot!

@jklymak jklymak mentioned this pull request Feb 5, 2021
Merged
@jklymak jklymak force-pushed the doc-impliment-reredirects branch 4 times, most recently from 8087a9d to 3af306c Compare February 6, 2021 18:46
@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Feb 6, 2021
@jklymak
Copy link
Member Author

jklymak commented Feb 6, 2021

As an example from the build above:
doc/api/backend_qt4agg_api.rst was removed in #19440

In https://github.com/matplotlib/matplotlib/pull/19456/files#diff-46354faebf416277aa00bb8daab45cde2fdbf1318ffdf1e5e1b0a468075798cd we say:

.. redirect-from:: /api/backend_qt4agg_api

So now if you navigate to:

https://53162-1385122-gh.circle-artifacts.com/0/doc/build/html/api/backend_qt4agg_api.html

you are immediately redirected to:

https://53162-1385122-gh.circle-artifacts.com/0/doc/build/html/api/backend_qt_api.html

@jklymak jklymak force-pushed the doc-impliment-reredirects branch 4 times, most recently from beec731 to 9289f61 Compare February 7, 2021 05:02
@anntzer
Copy link
Contributor

anntzer commented Feb 7, 2021 •
edited
Loading

Namely that moving whole directories will require each file in the directory getting this directive to its new location.

I guess you could support both .. redirect-from:: and a pre-set list of redirects in conf.py for glob redirects (that would just pre-populate the redirects mapping).

@jklymak jklymak force-pushed the doc-impliment-reredirects branch from 9289f61 to 4a205db Compare February 7, 2021 16:11
@jklymak
Copy link
Member Author

jklymak commented Feb 7, 2021

Namely that moving whole directories will require each file in the directory getting this directive to its new location.

I guess you could support both .. redirect-from:: and a pre-set list of redirects in conf.py for glob redirects (that would just pre-populate the redirects mapping).

Sure, well we could cross that bridge when we come to it. For now we have a pretty easy way to redirect if we move, so I think this can wait.

@jklymak jklymak marked this pull request as ready for review February 7, 2021 16:39
@jklymak jklymak added this to the v3.4.0 milestone Feb 7, 2021
@QuLogic
Copy link
Member

QuLogic commented Feb 11, 2021

@jklymak jklymak added the status: needs comment/discussion label 4 days ago

In the meeting, or is it okay to merge now?

@jklymak jklymak removed the status: needs comment/discussion needs consensus on next step label Feb 11, 2021
@jklymak
Copy link
Member Author

jklymak commented Feb 11, 2021

This is fine from my end. I think folks have had lots of time to voice concerns. Plus its not an irrevocable change if we decide this is a bad way to go.

@jklymak
Copy link
Member Author

jklymak commented Feb 11, 2021

@timhoffm @story645 @dstansby @brunobeltran This make sense to all of you?

@jklymak
Copy link
Member Author

jklymak commented Feb 11, 2021

Need to add warning for an overwrite....

@jklymak jklymak marked this pull request as draft February 11, 2021 21:23
@jklymak jklymak force-pushed the doc-impliment-reredirects branch 3 times, most recently from 7049b88 to cf7f0bb Compare February 11, 2021 22:08
@jklymak @anntzer @QuLogic
DOC: add redirects for new removed files
7e4a922 
ENH: add redirect_from sphinx extension

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jklymak jklymak force-pushed the doc-impliment-reredirects branch from cf7f0bb to 7e4a922 Compare February 11, 2021 23:43
@jklymak jklymak marked this pull request as ready for review February 12, 2021 00:23
@jklymak jklymak force-pushed the doc-impliment-reredirects branch from a00a2bf to 58f495a Compare February 15, 2021 21:30
@jklymak jklymak force-pushed the doc-impliment-reredirects branch from 58f495a to 47145d1 Compare February 16, 2021 00:43
@jklymak
Copy link
Member Author

jklymak commented Feb 16, 2021

.. added a redirect from /users/customizing to /tutorials/introductory/customizing as that was moved in #8545 (as discussed on gitter)

@tacaswell
Copy link
Member

Did we mean to take out examples/subplots_axes_and_figures/subplot_toolbar.py?

@jklymak
Copy link
Member Author

jklymak commented Feb 17, 2021

Did we mean to take out examples/subplots_axes_and_figures/subplot_toolbar.py?

Ooops! No, I mean we could ;-) It crashes on my machine when I do the doc build.

jklymak and others added 2 commits February 16, 2021 20:14
@jklymak
FIX
5425315
@jklymak @QuLogic
Update doc/sphinxext/redirect_from.py
8517b76 
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jklymak
Copy link
Member Author

jklymak commented Feb 23, 2021

I think this is still in good shape? I wouldn't mind it being merged so we can start doing more manual redirects

@anntzer anntzer merged commit 9bfd631 into matplotlib:master Feb 25, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 25, 2021
@anntzer @meeseeksmachine
Backport PR matplotlib#19456: Doc implement reredirects
3bcb006
@meeseeksmachine meeseeksmachine mentioned this pull request Feb 25, 2021
Merged
tacaswell added a commit that referenced this pull request Feb 25, 2021
@tacaswell
Merge pull request #19580 from meeseeksmachine/auto-backport-of-pr-19…
eb46343 
…456-on-v3.4.x

Backport PR #19456 on branch v3.4.x (Doc implement reredirects)
@anntzer anntzer mentioned this pull request Sep 2, 2021
Closed
@jklymak jklymak mentioned this pull request Jul 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@anntzer anntzer anntzer approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jklymak @QuLogic @anntzer @tacaswell @timhoffm