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

bug: mkdocstrings-python does not render Google-style Deprecated sections (missing template?) #227

Closed
the-13th-letter opened this issue Jan 16, 2025 · 10 comments
Assignees
Labels
bug Something isn't working extractor: griffe Related to griffe templates Jinja templates

Comments

@the-13th-letter
Copy link

Description of the bug

While writing up mkdocstrings/griffe#352, I noticed that when Griffe actually parses Google-style Deprecated sections, mkdocstrings-python (+ mkdocs-material) does not render them at all.

It seems that a Jinja template for Deprecated sections is missing (in the _base, material and readthedocs themes).

To Reproduce

Essentially the same as mkdocstrings/griffe#352. If mkdocstrings/griffe#353 is applied, then this Python function

def griffe_bug() -> None:
    """bla bla bla

    Deprecated:
        Since v1.0: Insecure! Do not use!
    """

renders as if it were actually written as

def griffe_bug() -> None:
    """bla bla bla"""

i.e. the "Deprecated" block is missing.

(If mkdocstrings/griffe#353 is not applied, then the block is rendered as an admonition... which is also wrong, albeit for different reasons. See mkdocstrings/griffe#352.)

Expected behavior

I'm not quite sure, because Deprecated blocks specifically expect a version number, and I don't know what you had originally envisioned here, @pawamoy. IMO something similar to

!!! warning "Deprecated"
    *version number*: other text of this section

(or maybe danger instead of warning) should work for most of us: it mimics the deprecation notices in Python's standard library.

Environment information

Debian Linux "testing", Python 3.12, mkdocstrings-python 1.13.0.

Additional context

mkdocstrings/griffe#352 and mkdocstrings/griffe#353

@the-13th-letter the-13th-letter added the unconfirmed This bug was not reproduced yet label Jan 16, 2025
@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

The deprecated sections in the Google-style mainly come from the equivalent warning in Numpydoc-style, see https://numpydoc.readthedocs.io/en/latest/format.html#deprecation-warning.

The version is supposed to be the exact version value, without additional text. What I envisioned is that the renderer would output something like "Deprecated in {version}", meaning users should not use Since "Dapper Drake", but only "Dapper Drake".

The issue with that is that we introduce English words and syntax ("in", order of words) that must then be translatable and translated. I'd prefer rendering something that is language-agnostic. Maybe {section_title} ({version})? WDYT?

"warning" sounds good. "danger" would be fine too, but warning seems best.

@pawamoy pawamoy added bug Something isn't working extractor: griffe Related to griffe templates Jinja templates and removed unconfirmed This bug was not reproduced yet labels Jan 16, 2025
@the-13th-letter
Copy link
Author

The deprecated sections in the Google-style mainly come from the equivalent warning in Numpydoc-style, see https://numpydoc.readthedocs.io/en/latest/format.html#deprecation-warning.

The version is supposed to be the exact version value, without additional text. What I envisioned is that the renderer would output something like "Deprecated in {version}", meaning users should not use Since "Dapper Drake", but only "Dapper Drake".

The issue with that is that we introduce English words and syntax ("in", order of words) that must then be translatable and translated.

I don't have any experience with Sphinx, and Numpydoc (un)helpfully doesn't show a typical Sphinx rendering of that markup. So I'll assume that Numpy's own docs are representative (example). That example appears to be generated with a .. deprecated: 2.1 header, which then gets translated into the leading text Deprecated since version {version}. It looks really good, but it does have that translation issue you bring up...

I'd prefer rendering something that is language-agnostic. Maybe {section_title} ({version})? WDYT?

{section_title} ({version}) looks good to me. But it bares another question: are we sticking to one "deprecated thing" per Deprecated section? Since there is no backwards compatibility concern, this may be the best time to (re)think about whether we'd want to parse this as a "multiple blocks" section or not.

In other words, if we have multiple deprecations, do we want this …

def deprecated_stuff() -> None:
    """A summary here.

    Deprecated:
        1.0: All arguments are deprecated.  (TODO: Add a really
        longform explanation for why.)

    Deprecated:
        1.1: Actually, do not use this at all!
    """

… or this …

def deprecated_stuff() -> None:
    """A summary here.

    Deprecated:
        1.0: All arguments are deprecated.  (TODO: Add a really
            longform explanation for why.)
        1.1: Actually, do not use this at all!
    """

… or even this?

def deprecated_stuff() -> None:
    """A summary here.

    Deprecated: 1.0
        All arguments are deprecated.  (TODO: Add a really
        longform explanation for why.)

    Deprecated: 1.1
        Actually, do not use this at all!
    """

I'd tend to prefer (2) over (1) or (3) aesthetically, but don't have a strong aesthetic preference of (1) vs. (3). (3) is probably error-prone to distinguish from an admonition, though.

"warning" sounds good. "danger" would be fine too, but warning seems best.

Fine by me as a default, but I'd wager this ought to be configurable (akin to how griffe-warnings-deprecated is configurable). I think this depends entirely on how "intense" the deprecation is: is it "we don't recommend you do this anymore", or is it "Danger! Danger! Do not touch this!"? That depends on the subject matter, and probably on the culture around the software/library that is being documented here.

@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

{section_title} ({version}) looks good to me.

OK lets do that then. We can always revise when the I18N/L10N is ready.

are we sticking to one "deprecated thing" ? Since there is no backwards compatibility concern, this may be the best time to (re)think about whether we'd want to parse this as a "multiple blocks" section or not.

Definitely. Honestly I'd be inclined to just entirely drop Deprecated sections as admonitions are generally more than enough for this:

Warning: Deprecated since 0.28:
    Some explanation on the deprecation.

Danger: Deprecated since 0.28:
    Some explanation on the deprecation.

But let say we keep the section anyway. In all three of your examples, what's the meaning of saying the thing is deprecated in 1.0, and deprecated again in 1.1? IMO it does not make sense, or at least I cannot find a use-case for it. That is, unless we do something a bit more advanced and let users deprecate parameters or other things, like the return value, etc.:

Deprecated:
    param1: Deprecated since v0.28, use `param2` instead.
    param3: Deprecated since v0.29, use `param4` instead.

But that will make the Google style diverge from Numpydoc. And that will necessitate a good spec. Is it worth it 😄?

Also yeah the third form clashes with custom titles for admonitions.

@pawamoy
Copy link
Member

pawamoy commented Jan 16, 2025

I have actually some work in progress in surveying the ecosystem for deprecation systems, as part of other work in Griffe for "API diffs". Let me tell you, the Python standard library and ecosystem are far, far from providing a comprehensive solution for all the things that can be deprecated in an API. So my feeling right now is: either we drop the Deprecated section, or we keep it really simple (like Numpy's), waiting for a more comprehensive solution (coming from me 😄?), or some documentation at least (in the form of a blog post or something else).

@the-13th-letter
Copy link
Author

[…] Honestly I'd be inclined to just entirely drop Deprecated sections as admonitions are generally more than enough for this:

Warning: Deprecated since 0.28:
    Some explanation on the deprecation.

Danger: Deprecated since 0.28:
    Some explanation on the deprecation.

I must admit I keep forgetting that this form of Google-style pseudo-section is supported, and that it is usually quite readable…

But let say we keep the section anyway. In all three of your examples, what's the meaning of saying the thing is deprecated in 1.0, and deprecated again in 1.1? IMO it does not make sense, or at least I cannot find a use-case for it. That is, unless we do something a bit more advanced and let users deprecate parameters or other things, like the return value, etc.:

Deprecated:
    param1: Deprecated since v0.28, use `param2` instead.
    param3: Deprecated since v0.29, use `param4` instead.

I would also just update the deprecated version marker if the deprecations overlap, but what about if they don't? It may still be the same function/class/module/attribute/whatever, but what if different aspects of it get deprecated independently, at different timelines? Functions with many parameters such as subprocess.Popen or json.dumps come to mind.

I think that the (multi-item) deprecated section is best used as a tabular or description list-like overview of deprecations that are explained in full detail in other sections of the documentation. Kind of like how a changelog is valuable because it is exhaustive, not because it is necessarily detailed.

I have actually some work in progress in surveying the ecosystem for deprecation systems, as part of other work in Griffe for "API diffs". Let me tell you, the Python standard library and ecosystem are far, far from providing a comprehensive solution for all the things that can be deprecated in an API.

While I do not know about machine consumers like Griffe, as far as human consumers go, I've personally never felt the Python standard library's way of documenting changes and deprecations to be unclear or hard to grep.

So my feeling right now is: either we drop the Deprecated section, or we keep it really simple (like Numpy's), waiting for a more comprehensive solution (coming from me 😄?), or some documentation at least (in the form of a blog post or something else).

Agreed that in such a case, you don't want to standardize anything prematurely. Because of mkdocstrings/griffe#352, we can be reasonably sure that no one is using the Deprecated section in production right now. (Or rather, no one is using it and not expecting to get a bog-standard admonition out of it.)

So, the safe thing to do now would be to "standardize" on a bog-standard warning-type admonition. The slightly less safe thing to do would be to turn it into the "Deprecated (version indicator)" warning-type admonition with the contents minus the version indicator part. If you implement this as a Jinja default template instead of a more "hardcoded" way, then this can even be amended later on if/when you have in-the-wild experience or feedback.

Either way, I'd recommend that Griffe's documentation and runtime warnings dissuade users from using it without in-the-wild experience or feedback. Oh, and not implement this configurability of admonition type I was mentioning earlier.

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2025

but what if different aspects of it get deprecated independently, at different timelines

Right 🤔 There we would want to be able to write this:

Deprecated:
    0.28: blabla
    0.29: other bla

...instead of:

Deprecated:
    0.28: blabla

Deprecated:
    0.29: other bla

However I probably wouldn't want the first example to render as a table or list (not visible enough in between other tables/lists). Instead I'd want it to render as 2 distinct warning/danger admonitions. Which feels a bit dissonant, compared to other sections and how they're rendered.

I've personally never felt the Python standard library's way of documenting changes and deprecations to be unclear or hard to grep.

Oh, I might have been unclear, sorry. It works perfectly fine indeed, I was more referring to libraries and decorators that actually let you mark objects as deprecated and attach a message to that. Stdlib now has warnings.deprecated, which handles a very limited set of deprecation kinds (classes, functions, methods, parameters when combined with typing.overload). There are many more and none of the libraries I found yet handle them in a comprehensive way.

The slightly less safe thing to do would be to turn it into the "Deprecated (version indicator)" warning-type admonition with the contents minus the version indicator part. If you implement this as a Jinja default template instead of a more "hardcoded" way, then this can even be amended later on if/when you have in-the-wild experience or feedback.

Yep, I'd go that route myself.

Either way, I'd recommend that Griffe's documentation and runtime warnings dissuade users from using it without in-the-wild experience or feedback.

Honestly makes me want to drop them. But for consistency with the Numpy parser, which actually parses them, lets enable them in the Google parser too. Unfortunately Numpy sections can't have a custom title so there's no admonition-based hack we can use with it. So for consistency again, if we can't recommend against in Numpy, lets not recommend against in Google either.


OK! I'll merge the Griffe PR, and then we can tackle the rendering part 🙂

@the-13th-letter
Copy link
Author

I've personally never felt the Python standard library's way of documenting changes and deprecations to be unclear or hard to grep.

Oh, I might have been unclear, sorry. It works perfectly fine indeed, I was more referring to libraries and decorators that actually let you mark objects as deprecated and attach a message to that. Stdlib now has warnings.deprecated, which handles a very limited set of deprecation kinds (classes, functions, methods, parameters when combined with typing.overload). There are many more and none of the libraries I found yet handle them in a comprehensive way.

So, the machine consumer side of things, then. You say warnings.deprecated is (or feels?) limited, but admittedly, the only thing I can think of that seems to be missing are attributes. If this isn't possible yet (I haven't checked), I can imagine that some form of cobbling properties and warnings.deprecated together should work here as well, sometime in the future.

But going beyond marking any use of a certain function/class/method/parameter/fixed parameter value/attribute/fixed attribute value as deprecated risks losing the guarantee that the deprecation status can still be computed in “useful“ time bounds.1 I'd definitely caution against giving the programmer too much freedom here: it's too easy for them to expect something that is computationally intensive (or even uncomputable in the general case) to be supported by your deprecation marking system.2

but what if different aspects of it get deprecated independently, at different timelines

Right 🤔 There we would want to be able to write this:

Deprecated:
    0.28: blabla
    0.29: other bla

...instead of:

Deprecated:
    0.28: blabla

Deprecated:
    0.29: other bla

However I probably wouldn't want the first example to render as a table or list (not visible enough in between other tables/lists). Instead I'd want it to render as 2 distinct warning/danger admonitions. Which feels a bit dissonant, compared to other sections and how they're rendered.

How about a table/list in an admonition? “Why choose one when you can have both?“ 😉

And taking Python's standard library as an example once more, the deprecated/changed notices are bundled at the bottom of each item's description, in a single paragraph. I'd reckon that putting each of them in a separate admonition is actually too visually overwhelming.

Either way, I'd recommend that Griffe's documentation and runtime warnings dissuade users from using it without in-the-wild experience or feedback.

Honestly makes me want to drop them. But for consistency with the Numpy parser, which actually parses them, lets enable them in the Google parser too. Unfortunately Numpy sections can't have a custom title so there's no admonition-based hack we can use with it. So for consistency again, if we can't recommend against in Numpy, lets not recommend against in Google either.

So out of curiosity, I checked how Numpydoc handles this. As far as I can tell (i.e., find numpydoc/ -name '*.py' -exec grep --context=3 -i deprecated '{}' +), Numpydoc never supported a section named “Deprecated”, not in 0.9.x nor in 1.0.0 nor right now; it's been Sphinx directives all along. And since Griffe never parsed Deprecated sections in Google-style docstrings properly, I conclude that there are zero backwards compatibility concerns to be concerned about. Technically.

So now would probably be The Right Time™ to change to a different design if that suits you better, while you are not yet constrained by backwards compatibility.

But in the end, you're the boss, boss.

Footnotes

  1. Usually meaning “constant“, but perhaps “linear“, “quadratic“ or even “polynomial“ may be acceptable here.

  2. Just off the top of my head: “all Python code objects that contain infinite loops are deprecated“. That is the halting problem for Python code objects, so it's uncomputable. And you really don't want to make the empty promise that your API inspection system can handle this setup. So you'd better be really really sure that the programmer cannot express this deprecation criteria in your API inspection system.

@pawamoy
Copy link
Member

pawamoy commented Jan 17, 2025

“all Python code objects that contain infinite loops are deprecated“

No need to go that far haha, here are a few from the top of my head (collapsed as this is getting off-topic):

what can be deprecated?
  • argument types/values
  • parameter kinds (positional-or-keyword to positional-only / keyword-only, variadic to something else, etc.)
  • parameter default values
  • returned types/values (hard to check on the user side)
  • raised exceptions (caught exceptions on the user side, hard to check)
  • logger names (yep, people rely on them)
  • logger messages if we like to hurt ourselves
  • changes in classes MRO
  • probably a few more things

How about a table/list in an admonition? “Why choose one when you can have both?“ 😉

Heh, right again!

Numpydoc never supported a section named “Deprecated”,

True, Griffe takes a few liberties to try its best and unify the different styles under a common denominator: the data structures that hold the info.

Aaaaaaaaah... maybe I shouldn't try too hard and decide that if Numpydoc doesn't specify a deprecated section, then Griffe won't support one.

Seeing how much back and forth is needed to decide how to even render such section kinda is the final drop for me: I think I'll entirely remove support for Deprecated sections 🤔 I would revert the changes in Griffe (so sorry I made you update your changes only to revert them later 🙇), and close this issue as won't fix 🤔 Then if users want them back for some reason, we'll tell them to ask Google and Numpy to revise their specs style guides.

@the-13th-letter
Copy link
Author

what can be deprecated?
  • argument types/values
  • parameter kinds (positional-or-keyword to positional-only / keyword-only, variadic to something else, etc.)
  • parameter default values
  • returned types/values (hard to check on the user side)
  • raised exceptions (caught exceptions on the user side, hard to check)
  • logger names (yep, people rely on them)
  • logger messages if we like to hurt ourselves
  • changes in classes MRO
  • probably a few more things

Ah, true, I should probably have come up with (4) and (5) myself, and I can see how everything beyond (4) is currently impossible to mark.

Seeing how much back and forth is needed to decide how to even render such section kinda is the final drop for me: I think I'll entirely remove support for Deprecated sections 🤔 I would revert the changes in Griffe (so sorry I made you update your changes only to revert them later 🙇), and close this issue as won't fix 🤔 Then if users want them back for some reason, we'll tell them to ask Google and Numpy to revise their specs style guides.

Not a problem. This turned out to be more of a design problem than a code problem, and you did highlight sensible admonition replacements for Deprecated sections via custom Warning and Danger sections. That works for me. 😄

@pawamoy
Copy link
Member

pawamoy commented Jan 18, 2025

Thank you 🙇 What motivates me is that we could actually bring custom titles to the Numpy parser too, by simply splitting on : (like we do in the Google parser), meaning we'd have this admonition solution in both parsers. OK, closing as won't fix. Thank you very much for your patience on your help on this matter 🙂

@pawamoy pawamoy closed this as not planned Won't fix, can't repro, duplicate, stale Jan 18, 2025
the-13th-letter added a commit to the-13th-letter/derivepassphrase that referenced this issue Jan 19, 2025
The Deprecated section has always been non-standard, and
`mkdocstrings-python` recently removed explicit support for this.  The
recommended alternative is an explicit admonition in section notation
(i.e., a section named Warning, or Danger, or whatever), which is still
very readable in the docstring and conveys a clear intention.

References:
[mkdocstrings/python#227](mkdocstrings/python#227).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working extractor: griffe Related to griffe templates Jinja templates
Projects
None yet
Development

No branches or pull requests

2 participants