-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: Implement javascript to redirect old whatsnew pages to new ones #23708
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
Conversation
function reformatVersion(version) { | ||
return "v" + version.slice(0,1) + "." + version.slice(1,-1) + "." + version.slice(-1) + ".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.
Newline at end of file.
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.
Also, are these whatsnew
functions parameterized on the version of the whatsnew
that we are parsing?
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.
So the function assumes the format according to all the other links (#23695) and assumes the version number is vX.X(X).X; so 1 digit, 1 or 2 digits and then 1 digit. Do you think it should be more general?
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, sorry, so this wasn't 100% clear. When I said "parameterized on version," I was referring to this comment in the original issue:
We should be able to detect this in Javascript. When we're on the whatsnew for 0.24.0 or greater, we want to look for anchors pointing to /whatsnew.html#whatsnew- for 0.23.4 or older. Then we'd rewrite /whatsnew.html#whatsnew--<title> to /whatsnew/.html#<title>.
So this is referring to your script in general. I didn't see any checks for version?
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.
Ahh OK I missed this part, I shall amend this!
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 have a fix to check for the version number, but since I use a regex to check if the link format is '#whatsnew-XXX-...' before attempting to change the URL, this should prevent this issue, since all links with v0.23.5 or greater will be of the new format of 'v.0.28.9-enhancements...' and so would fail the regex match anyways?
If that's not the case, I'll push this latest fix to check for version number and only amend links that are v0.23.4 or older.
@ismailuddin : Can you rebase your PR on |
Hello @ismailuddin! Thanks for updating the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23708 +/- ##
==========================================
+ Coverage 92.24% 92.25% +<.01%
==========================================
Files 161 161
Lines 51336 51383 +47
==========================================
+ Hits 47357 47404 +47
Misses 3979 3979
Continue to review full report at Codecov.
|
58157c7
to
01f5905
Compare
Fix function name typo Co-Authored-By: ismailuddin <[email protected]>
Just a bump regarding @gfyoung 's comment about checking version; do I need to check the version number since all new links will be of the new format and hence not match the regex pattern? Or will they not be? See above for discussion.. |
IIUC, you don't need to worry about @gfyoung's concern. We aren't rebuilding the old docs with this JS applied. |
Just tried this out locally, didn't quite work. For a URL like
I basically get a 404, since the page |
@ismailuddin do you have time to review the mentioned problem, and see if you can fix it. |
@datapythonista Ah yes sorry, I thought Tom was asking someone also about this. I'll look into it! OK, so if I'm understanding this correctly, the broken links are arising due to old links let's say on Google pointing to this
This approach would only change the /stable/ links which point to the old whatsnew.html pages, which should be the only problem? |
didn't check in detail, but what I understand is that even for old version of pandas, we changed the urls in this way:
So, we need to add javascript code to |
@datapythonista I believe that's correct. Though note that there isn't a |
Implementing it shouldn't be difficult, we can just add the We already have the system for the API redirects, but that won't work, even if we make it generic to handle non-api pages, as in this case we have a 1 to N redirect. But personally, I'd prefer to have a nice 404 page, with a link to the home page, and not add more complexity with redirects. I think we should reorganize several pages (have a directory for the user guide, another for the contributor docs...), and having redirects for those changes will be quite complex. |
Do you expect this to be complex? This one is quite limited in scope I
think.
…On Thu, Dec 20, 2018 at 4:58 PM Marc Garcia ***@***.***> wrote:
Implementing it shouldn't be difficult, we can just add the whatsnew.html
to html_additional_pages in doc/source/conf.py, and have the javascript
in its content.
We already have the system for the API redirects, but that won't work,
even if we make it generic to handle non-api pages, as in this case we have
a 1 to N redirect.
But personally, I'd prefer to have a nice 404 page, with a link to the
home page, and not add more complexity with redirects. I think we should
reorganize several pages (have a directory for the user guide, another for
the contributor docs...), and having redirects for those changes will be
quite complex.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#23708 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIh5P5tVt_B5sxi0nZZcBl0W2cnqNks5u7BYIgaJpZM4Yepgt>
.
|
No, not this particular case, I think redirecting the whatsnew it's quite simple, what I described in my last comment. What I think will be complex enough to not be worth, is redirecting if we reorganize the documentation. So, I was wondering if it was worth adding redirects every time we move pages, or just improve the 404, and let the users find the new content. |
Ahh yes I see what you mean, and I think I agree.
…On Fri, Dec 21, 2018 at 9:35 AM Marc Garcia ***@***.***> wrote:
No, not this particular case, I think redirecting the whatsnew it's quite
simple, what I described in my last comment. What I think will be complex
enough to not be worth, is redirecting if we reorganize the documentation.
So, I was wondering if it was worth adding redirects every time we move
pages, or just improve the 404, and let the users find the new content.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#23708 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABQHIn0YY9TwniDkYQ8qPqci4w1TRmKBks5u7P_PgaJpZM4Yepgt>
.
|
Closing this, as it's not solving the problem. @ismailuddin , as discussed, not sure if it's worth to add the redirects. But feel free to open a new PR (or reopen this), if you want to implement the approach described in #23708 (comment) Thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff
Javascript file to reformat / redirect 'whatsnew' href links in documentation.