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

docs: add example Juju version markers #1311

Merged
merged 11 commits into from
Aug 20, 2024

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Aug 8, 2024

Adds the version features were added in Juju to the API reference docs (other than ops.testing).

No version tags are added for anything older than 3.0 - meaning that if there's no version tag, then it was available in the oldest supported Juju version.

Unfortunately, the built-in Sphinx versionadded directive does not support customising the text, so while otherwise suitable, it results in "Added in version 3.0" (seems like it's referring to ops 3.0) or "Added in version Juju 3.0". Instead, a new pair of custom directives are added, jujuversion and jujuremoved. These are very heavily based on versionadded and versionremoved but have custom text (and we could customise them further, e.g. linking to revision notes) if we wanted to in future). In order to avoid having to also add custom CSS, they use the same classes as the versionadded and versionremoved directives.

The features being removed in Juju 4.0 also have jujuremoved tags added.

I also ran ruff format over custom_conf.py, which is why there are some unrelated formatting changes.

Fixes #1305

Copy link
Contributor

@dimaqq dimaqq left a comment

Choose a reason for hiding this comment

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

I'm very happy to see this, our users ought to be delighted.

If we wanted to be pedantic, I'd go for this syntax:

    .. versionadded:: 2.15
       Requires Juju 3.5

Listing both this library version and Juju version.

ops/charm.py Outdated Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor Author

If we wanted to be pedantic, I'd go for this syntax:

    .. versionadded:: 2.15
       Requires Juju 3.5

Listing both this library version and Juju version.

We did previously have some (definitely not complete, and generally using text, not the directive) markers of when functionality was added in ops. We removed that and adjusted the CHANGES file just a few months ago.

I do agree that it's both providing the most information and looks nice:

image

The main reason the ops version markers were removed (rather than extended) was to avoid people looking through to find a specific version of ops to use rather than just always using the latest version. I think a secondary one was that over time this does get quite noisy because almost everything ends up with a little tag. We also considered versioned API docs but decided that it would be best to only do that for major versions (ie. we should have both 2.{latest} and 3.{latest} versions if/when there's an ops 3).

If we do use this format, then it becomes less consistent to not have the markers on ops features that aren't also new Juju features (so almost everything ends up with a version label).

@dimaqq
Copy link
Contributor

dimaqq commented Aug 8, 2024

docs build failed, it seems?

https://readthedocs.org/projects/ops/builds/25247433/

@dimaqq
Copy link
Contributor

dimaqq commented Aug 8, 2024

Unknown directive type "versionremoved"

@tonyandrewmeyer
Copy link
Contributor Author

Unknown directive type "versionremoved"

Yeah, that's this:

(Also in version changes: newer Sphinx have versionremoved, which I've experimented with to see how that would look for the ones being removed in 4.0. With current Sphinx that generates an error, so I'll remove it before moving this PR from draft, but I'm leaving it in place in case people would like to examine that too).

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review August 13, 2024 22:00
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Love it, thanks! A few comments, mainly wondering if we should remove the notes about Juju 4.0, which won't be shipped for many months yet.

docs/custom_conf.py Show resolved Hide resolved
docs/custom_conf.py Show resolved Hide resolved
ops/charm.py Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
@@ -334,3 +330,85 @@ def _compute_navigation_tree(context):
('py:class', 'ops.testing.CharmType'),
('py:obj', 'ops.testing.CharmType'),
]


# This is very strongly based on
Copy link
Contributor

Choose a reason for hiding this comment

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

@tonyandrewmeyer tonyandrewmeyer merged commit e9215eb into canonical:main Aug 20, 2024
27 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the version-of-juju-1305 branch August 20, 2024 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add minimum juju version to event docstring
3 participants