Conversation
|
Work by @qt1p |
humitos
left a comment
humitos
left a comment
There was a problem hiding this comment.
It looks good! I left some questions to understand better how this feature is used.
The configuration page from the documentation should be updated as well (https://sphinx-version-warning.readthedocs.io/en/latest/configuration.html) to reflect these changes.
| } | ||
| else if (config.meta.check_version_fn) { | ||
| // A custom function for checking version is defined globally | ||
| var fn = window[config.meta.check_version_fn]; |
There was a problem hiding this comment.
How does this work? Where is the function check_version_fn defined`?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@qt1p What do we have to do in sphinx config to use check_version?
| 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') |
There was a problem hiding this comment.
What would be a valid value for versionwarning_check_version_fn? Is just the name of the function?
| else if (config.meta.check_version_fn) { | ||
| // A custom function for checking version is defined globally | ||
| var fn = window[config.meta.check_version_fn]; | ||
| fn(config, showBanner); |
There was a problem hiding this comment.
Why is needed to pass showBanner here?
There was a problem hiding this comment.
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.
| data = json.dumps({ | ||
| 'meta': { | ||
| 'api_url': config.versionwarning_api_url, | ||
| 'check_version_fn': config.versionwarning_check_version_fn, |
There was a problem hiding this comment.
I would use something more explicit, like versionwarning_js_check_version_function
There was a problem hiding this comment.
Sure, we can definitely change the name of the param.
qt1p
force-pushed
the
optional-checkversion-func
branch
2 times, most recently
from
ccc5bf2 to
832fc04
Compare
qt1p
force-pushed
the
optional-checkversion-func
branch
from
832fc04 to
e716b6d
Compare
4 participants
This should fix #29