[WAIT TO MERGE] Store deprecation metadata on functions for docs generation #9611
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4283174916
💛 - Coveralls |
|
I forgot the full context: does this need a rebase over #9616, or was it a partial attempt that we decided to go a different way on? |
…ore-deprecation-metadata
Eric-Arellano
changed the title
Fixed now. I do this pattern a lot: "prework PRs", where I separate out refactors from the actual feature change. It makes reviews smaller and easier to understand, and reduces risk if we need to revert something. This is now ready to land, so long as we are okay with the approach taken in #1680 with how to handle deprecated arguments, which Luciano and I weren't sure about at first. But him and I reached consensus. See the details of that PR description. |
Eric-Arellano
requested review from
a team,
manoelmarques and
woodsp-ibm
as code owners
…ore-deprecation-metadata
…ore-deprecation-metadata
| func_name = func.__name__ | ||
| old_kwarg_to_msg = {} | ||
| for old_arg, new_arg in kwarg_map.items(): | ||
| msg_suffix = ( | ||
| "will in the future be removed." if new_arg is None else f"replaced with {new_arg}." | ||
| ) | ||
| old_kwarg_to_msg[ | ||
| old_arg | ||
| ] = f"{func_name} keyword argument {old_arg} is deprecated and {msg_suffix}" |
There was a problem hiding this comment.
This logic needs to live here, rather than the helper function _rename_kwargs, because we need to call _DeprecationMetadataEntry().store_on_function() on the wrapper rather than the inner func
| category: Type[Warning] = DeprecationWarning, | ||
| ) -> None: | ||
| for old_arg, new_arg in kwarg_map.items(): | ||
| if old_arg in kwargs: |
There was a problem hiding this comment.
I inverted the conditional to early return via continue if this is False. It results in less nesting.
| del old_arg | ||
| del new_arg |
There was a problem hiding this comment.
Are these lines added here in order to pass linting?
There was a problem hiding this comment.
Exactly. Pylint is overly pedantic
| del new_arg | ||
|
|
||
| self.assertEqual( | ||
| getattr(my_func, _DeprecationMetadataEntry.dunder_name), |
There was a problem hiding this comment.
Will this return the whole list of deprecation metadata associated with the deprecated function and deprecated arguments?
There was a problem hiding this comment.
Yeah, the attribute __qiskit_deprecations__ stores a List[_DeprecationMetadataEntry] on the function object. This is key, it allows us to store >1 deprecation on a function, e.g. if multiple args have been deprecated.
Eric-Arellano
changed the title
|
Superseded by #9685. |
Summary
Rework of #8600 and part of #1680.
Our goal is to start putting
.. deprecated ::directives in our docs. But, we don't want to ask developers to manually put those in the docstring, as that would duplicate the@deprecated_*decorators we already have for runtime warnings.So, we keep the decorators, but have them now store structured metadata by adding a
__qiskit_deprecations__attribute to relevant function objects. Then, the Sphinx extension from #1680 will look for this attribute and add the documentation.Details and comments
Why not take the approach of #8600, where we dynamically change the
__doc__of each class to have the deprecation block inside it? It resulted in too much of a performance hit at runtime.Why store the data in
__qiskit_deprecations__, rather than Sphinx learning how to parse the decorators? a) It would be harder to get the parsing code working, and b) it is too brittle, e.g. not working with us adding future decorators or how Nature has its own decorators. Instead, we have a standardized and structured way for anyone to set this metadata.