Skip to content
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

refactor: separate deprecated config for readability #628

Merged

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jun 30, 2023

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:

  • relocated _deprecated_oauth_aliases list of deprecated config and the deprecated/removed config to the end of config declarations
  • reduced help strings of deprecated/removed config to "[Deprecated|Removed], use :attr:`.<class>.<new_config>`" strings

Code refactoring preview

image

Config reference docs preview

image

@consideRatio consideRatio force-pushed the pr/isolate-deprecation-logic branch 2 times, most recently from ebffc5f to 41f3d09 Compare June 30, 2023 08:05
@consideRatio consideRatio self-assigned this Jun 30, 2023
@consideRatio consideRatio marked this pull request as ready for review June 30, 2023 20:42
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

LGTM, one comment about making sure version numbers are in deprecation messages

help="""
Allow members of selected Bitbucket teams to sign in.
""",
help="Deprecated, use :attr:`.BitbucketOAuthenticator.allowed_teams`.",
Copy link
Member

Choose a reason for hiding this comment

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

I've learned that it's a good practice to always have in VERSION everywhere you have a 'deprecated' or 'removed' message, so that folks who see it know how to coordinate the deprecations with their version requirements without having to dig around and find the relevant version where things changed.

@consideRatio
Copy link
Member Author

consideRatio commented Jul 3, 2023

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

  • we should use the .. deprecated:: <version> admonition?
    • if so, what to do about the "Removed in .."?
  • we should write a prefix v in this context?

@minrk
Copy link
Member

minrk commented Jul 3, 2023

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
Copy link
Member Author

Thanks @minrk! I've opened sphinx-doc/sphinx#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
Copy link
Member Author

consideRatio commented Jul 3, 2023

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
Copy link
Member

minrk commented Jul 3, 2023

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 consideRatio requested a review from minrk July 3, 2023 14:47
@consideRatio consideRatio merged commit e926e90 into jupyterhub:main Jul 3, 2023
10 checks passed
@consideRatio
Copy link
Member Author

Thanks for reviewing @minrk!!!

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.

None yet

2 participants