Skip to content

Create optional checkVersion function #31

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

Open
ntolia wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Create optional checkVersion function #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion versionwarning/_static/js/versionwarning.js
View file
Open in desktop

Large diffs are not rendered by default.

24 changes: 18 additions & 6 deletions versionwarning/_static/js/versionwarning.src.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ function injectVersionWarningBanner(running_version, version, config) {
console.debug("injectVersionWarningBanner");
var version_url = window.location.pathname.replace(running_version.slug, version.slug);
var warning = $(config.banner.html);
if (config.meta.check_version_fn) {
version_url = config.meta.api_url;
}

warning
.find("a")
Expand Down Expand Up @@ -43,6 +46,15 @@ function getHighestVersion(versions) {
return highest_version;
}

function showBanner(running_version, highest_version, config) {
console.debug("showBanner");
if (
semver.valid(semver.coerce(running_version.slug)) && semver.valid(semver.coerce(highest_version.slug)) &&
semver.lt(semver.coerce(running_version.slug), semver.coerce(highest_version.slug))) {
console.debug("Highest version: " + highest_version.slug);
injectVersionWarningBanner(running_version, highest_version, config);
}
}

function checkVersion(config) {
console.debug("checkVersion");
Expand All @@ -67,12 +79,7 @@ function checkVersion(config) {
success: function (versions) {
// TODO: fetch more versions if there are more pages (next)
highest_version = getHighestVersion(versions["results"]);
if (
semver.valid(semver.coerce(running_version.slug)) && semver.valid(semver.coerce(highest_version.slug)) &&
semver.lt(semver.coerce(running_version.slug), semver.coerce(highest_version.slug))) {
console.debug("Highest version: " + highest_version.slug);
injectVersionWarningBanner(running_version, highest_version, config);
}
showBanner(running_version, highest_version, config);
},
error: function () {
console.error("Error loading Read the Docs active versions.");
Expand All @@ -97,6 +104,11 @@ function init() {
else if (config.banner.custom) {
injectCustomWarningBanner(config);
}
else if (config.meta.check_version_fn) {
// A custom function for checking version is defined globally
var fn = window[config.meta.check_version_fn];
Copy link
Owner

@humitos humitos Jun 30, 2020

Choose a reason for hiding this comment

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

How does this work? Where is the function check_version_fn defined`?

Copy link

@qt1p qt1p Jun 30, 2020

Choose a reason for hiding this comment

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

Apologies @humitos I should have added more detailed comments.
This function is defined globally, which in the browser is the window object. We define it in our sphinx docs config in custom.js. check_version_fn simply contains the name of the function and this line of code creates a reference to the function which we invoke on the following line.

Copy link

@Rjvs Rjvs Jun 6, 2022

Choose a reason for hiding this comment

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

@qt1p What do we have to do in sphinx config to use check_version?

fn(config, showBanner);
Copy link
Owner

@humitos humitos Jun 30, 2020

Choose a reason for hiding this comment

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

Why is needed to pass showBanner here?

Copy link

@qt1p qt1p Jun 30, 2020

Choose a reason for hiding this comment

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

The reason we weren't able to use the plugin as-is was that our docs are not hosted with RTD and we need to use a different mechanism for fetching and comparing our current version. So I refactored the comparison logic into the showBanner function.

And since our custom function is not defined in the same namespace as showBanner (it's defined on window in our custom.js file) we must pass the showBanner function along so that our function can call it with the params running_version and highest_version.

Sorry for the complexity @humitos. I'm open to other ideas for providing a custom function.

}
else {
checkVersion(config);
}
Expand Down
1 change: 1 addition & 0 deletions versionwarning/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def setup(app):
app.add_config_value('versionwarning_body_selector', 'div.body', 'html')
app.add_config_value('versionwarning_project_slug', os.environ.get('READTHEDOCS_PROJECT', None), 'html')
app.add_config_value('versionwarning_project_version', os.environ.get('READTHEDOCS_VERSION', None), 'html')
app.add_config_value('versionwarning_check_version_fn', '', 'html')
Copy link
Owner

@humitos humitos Jun 30, 2020

Choose a reason for hiding this comment

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

What would be a valid value for versionwarning_check_version_fn? Is just the name of the function?


if sphinx.version_info >= (1, 8):
# ``config-initied`` requires Sphinx >= 1.8
Expand Down
1 change: 1 addition & 0 deletions versionwarning/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def generate_versionwarning_data_json(app, config=None, **kwargs):
data = json.dumps({
'meta': {
'api_url': config.versionwarning_api_url,
'check_version_fn': config.versionwarning_check_version_fn,
Copy link
Owner

@humitos humitos Jun 30, 2020

Choose a reason for hiding this comment

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

I would use something more explicit, like versionwarning_js_check_version_function

Copy link

@qt1p qt1p Jun 30, 2020

Choose a reason for hiding this comment

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

Sure, we can definitely change the name of the param.

},
'banner': {
'html': banner_html,
Expand Down