Skip to content

Conversation

@thisisnic
Copy link
Member

@thisisnic thisisnic commented Jan 17, 2022

Implemented:

  • Version switcher replaces package version on pkgdown
  • Package versions and URLs stored in arrow/r/pkgdown/assets/versions.json
  • Tested with a web browser and JS console
  • functionality to check URL can be retrieved and redirect to root dir if not (thanks @jorisvandenbossche for your previous work on this which constitutes 90% of this!)

Suggestion for testing without putting together a complex Docker script:

  • copy script from extra.js
  • as the versions.json file is not deployed yet, replace ./versions.json in line $.getJSON("./versions.json", function( data ) { with https://raw.githubusercontent.com/thisisnic/arrow/ARROW-14338_pkgdown_versioning/r/pkgdown/assets/versions.json
  • open R docs page and paste into JS console

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic thisisnic marked this pull request as ready for review January 18, 2022 10:14
@kszucs kszucs force-pushed the ARROW-14338_pkgdown_versioning branch from 82f5c77 to e219f6d Compare January 18, 2022 10:44
@thisisnic thisisnic requested a review from ianmcook January 18, 2022 19:36
@kszucs kszucs requested a review from jonkeane January 18, 2022 19:50
@jorisvandenbossche
Copy link
Member

Cool! The screenshot looks good. I don't have time to test it right now, but if it is working for you locally, I trust that, and I would merge it and then we can still see how it works in the hosted dev docs

thanks @jorisvandenbossche for your previous work on this which constitutes 90% of this!)

Well, thanks to the person that implemented it in the pydata-sphinx-theme, as I just copied it from there :)
(and I should actually update it to use the upstream version, now it is released)

@kszucs
Copy link
Member

kszucs commented Jan 19, 2022

Including to 7.0.

@jorisvandenbossche
Copy link
Member

One question though: where does this versions.json asset get included? (where does it get copied on the actual website)
Because we should ensure that for every version, it is pointing to the same root version of that file, so we can update the versions in that file after the docs for a given version are built.

}

// Load the versions JSON and construct the select items
$.getJSON("./versions.json", function( data ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "./versions.json", is that a relative path to the current location? Because that might not work if you have sub-directories in the docs (like for the articles)

Or does that point to a "root" file? (in which case it would always be arrow.apache.org/versions.json once deployed, which might also not be correct?)

Copy link
Member Author

@thisisnic thisisnic Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; I guess it'd make the most sense to hard code it to point to the published version like this?

Suggested change
$.getJSON("./versions.json", function( data ) {
$.getJSON("https://arrow.apache.org/docs/r/versions.json", function( data ) {

Or perhaps not if the docs are published somewhere else though. I guess I need to construct the URL...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this part is annoying to test with an absolute url before it's actually deployed .. :)

But, I think an absolute url is probably the easiest for now. I would maybe not put it in /docs/r/versions.json, as that is the stable docs location (it would get overwritten when doing a release and recreating the stable docs). Unless the file is not included in the actual arrow repo, and only in the arrow-site repo. Although I see now that I actually did the same for the sphinx site ... :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added another commit where I did it a little differently to construct a relative URL based on the fact that the only thing we know for sure is the structure that pkgdown has means that the final instance of /r/in the path will be where versions.json is hosted. I think it makes sense now!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So does that mean that if you are on a page like https://arrow.apache.org/docs/5.0/r/articles/developing.html, that would point to https://arrow.apache.org/docs/5.0/r/versions.json? (taking the last /r, and appending versions.json)

Because then it still points to the versions.json of that version, and not the latest one (which can be updated)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, of course! Reverting to absolute URL...

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @thisisnic! Merging on green.

@kszucs kszucs closed this in 9938f70 Jan 19, 2022
@ursabot
Copy link

ursabot commented Jan 19, 2022

Benchmark runs are scheduled for baseline = ff4b9be and contender = 9938f70. 9938f70 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants