jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
413 stars 365 forks source link

refactor: separate deprecated config for readability #628

Closed consideRatio closed 1 year ago

consideRatio commented 1 year ago

Some classes had quite a bit of no longer relevant config, and since they also had long help strings it was a bit tricky to overview effectively what was relevant. With this PR I've:

Code refactoring preview

image

Config reference docs preview

image

consideRatio commented 1 year ago

Thanks for reviewing @minrk! I was thinking "should I write "Deprecated in 0.12" or "Deprecated in v0.12" and searched for examples in jupyterhub/jupyterhub, where I found the following:

image image

@minrk do you think

minrk commented 1 year ago

we should use the .. deprecated:: admonition?

Yeah, since it gets rendered in docs, that makes sense.

if so, what to do about the "Removed in .."?

Hm. A few options - you could use the deprecated admonition and put "this has been removed" in the body, like:

.. deprecated:: 1.2
    Using this option will result in an error, ...

or use versionchanged with the same explanation.

or even try this extension so we could use .. versionremoved::.

we should write a prefix v in this context?

No, v is not part of the version, it's an abbreviation used some places (e.g. in a git tag) for the word 'version' to put next to the actual version number. So version v16 would be saying 'version version 16'.

consideRatio commented 1 year ago

Thanks @minrk! I've opened https://github.com/sphinx-doc/sphinx/issues/11480 about .. versionremoved btw, it seems reasonable for the sphinx project to consider adding that to complement the other directives to capture the full lifecycle of things.

consideRatio commented 1 year ago

I've concluded that if we use an extension like that, it can be done without installing a new dependency with this in conf.py.

# -- Add versionremoved directive ---------------------------------------------------
# ref: https://github.com/sphinx-doc/sphinx/issues/11480
#
from sphinx.domains.changeset import VersionChange, versionlabel_classes, versionlabels

def setup(app):
    if "versionremoved" not in versionlabels:
        versionlabels["versionremoved"] = "Removed in version %s"
        versionlabel_classes["versionremoved"] = "removed"
        app.add_directive("versionremoved", VersionChange)

The downside is that it doesn't render nicely, because its theme dependent as well based on this in pydata-sphinx-theme.

image

minrk commented 1 year ago

Since it's simple like that, I think it's fine. Good find! It's probably not that often that we'll have attributes that are still present and documented but 'removed'.

consideRatio commented 1 year ago

Thanks for reviewing @minrk!!!