-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: Include alternatives to deprecations in the documentation #22038
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
Conversation
383d68f
to
f308421
Compare
I think this is 💯 from a functionality point of view, but I am a bit worried about inventing a new link markup scheme and the re-writing to rst. Can we just say "this is standard rst that will be rendered" and let the rst leak out to the in-line error message? If we think that is unacceptable, I would rather add another argument to the deprecation tools to allow what shows up in the documentation to be customized (rather than just dropping the value of alternative in). Text processing is hard in general and in my experience text-processing via regex can be very brittle, accepting a bit of duplication in code that is explicitly going to be deleted is probably less work overall than understanding some (admittedly clever) text mutation rules. |
Maybe I should have better described the PR as "Allow rst in
There is no new scheme or any re-writing to rst, everything is standard rst. The
This is certainly true and I didn't mean to cover all possible corner cases when converting the rst into plain text for the warning. Complex rst expressions with nested and escaped Possible alternatives:
|
f308421
to
c97c41f
Compare
I just saw that the space between the caption and the link is optional (even not recommended although the examples in the rst spec contain spaces), so I changed the wording in the docs, the regex and the alternatives with separate captions accordingly. I also changed the test a bit to make it easier to read. |
I misunderstood what was going on 🐑 . I thought that the transform was on the way into the rst not on the way into the warning text. I stand by my logic, but it is not relevant to this PR. I do not think option 1 is terrible as even though it might look a bit funny, it does give more information about what is going on (by pointing to the class / method of interest). We have had some issues in the past where the alternative made perfect sense to us when we reviewed the PR (because we had the context of what class was being edited), it made no sense to users (because the class the method that was suggested as an alternative was on was not clear). It would be a shame to have that information in the source and documentation, but not visible at run time. |
c97c41f
to
fcbc73f
Compare
This is amazing! I'm very pro overall. I'm also on the side of option 1 from #22038 (comment). I think the RST formatted text (at least in all the cases and types used in this PR) is parseable as plain text, and the removal of the RST markup isn't worth the hassle of maintaining a rather complex regular expression substitutuion. |
I removed the rst text processing and pushed it as a separate commit (so it could be reverted easily in case someone complains about strange deprecation warning messages). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably didn't need to bother with the 3.4 deprecations as they're about to be (or already) deleted.
9b4ade5
to
40580ac
Compare
- add `alternative` and `addendum` to the documentation by including them in the second argument of the `.. deprecated::` directive - allow for backticks and optional reference to create links to the mentioned alternatives in the documentation - remove these additional links and backticks in the deprecation warning message that is emitted when using a deprecated item Example: @_api.deprecated("3.5", alternative="`num2date(e).timestamp()<.num2date>`" will create the directive .. deprecated:: 3.5 Use `num2date(e).timestamp()<.num2date>` instead. which creates a link to matplotlib.dates.num2date with the caption num2date(e).timestamp(). The deprecation warning message will read "... Use num2date(e).timestamp() instead."
where possible
to convert rst to plain text for warnings. This reverts the rst text processing of commmit "Include alternatives to deprecations in the documentation" as it was considered to be too complex to maintain or potentially error prone.
40580ac
to
baa99d3
Compare
Thank you @StefRe ! |
OK, so this breaks the docs build if I have:
With |
You may try |
But that looks terrible in the actual warning? |
@jklymak I think enclosing it in backticks |
@meeseeksdev backport to 3.5.x |
Something went wrong ... Please have a look at my logs. It seems that the branch you are trying to backport to does not exist. |
@meeseeksdev backport to v3.5.x |
…ns in the documentation
…038-on-v3.5.x Backport PR #22038 on branch v3.5.x (DOC: Include alternatives to deprecations in the documentation)
PR Summary
There are 90 deprecations currently, for 38 of which alternatives are given in the deprecation warning. These alternatives are not included, however, in the documentation.
The first commit enables inclusion of alternatives along with links in the documentation and the second commit adds links to the documentation for the existing alternatives if they refer to another function or class.
Current:
Documentation:
New:
Documentation:
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).