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

Uppercase Qiskit in deprecation warnings #13452

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Nov 18, 2024

Summary

This is a suggestion to change the default deprecation message to use Qiskit 1.x instead of qiskit 1.x, since we commonly refer to this package in uppercase spelling.

Details and comments

This means the message is changed e.g. from

The property ``qiskit.dagcircuit.dagcircuit.DAGCircuit.unit`` is deprecated as of qiskit 1.3.0.

to

The property ``qiskit.dagcircuit.dagcircuit.DAGCircuit.unit`` is deprecated as of Qiskit 1.3.0.

The old format can still be obtained by explicitly setting package_name="qiskit" in the deprecation decorators.

@Cryoris Cryoris requested a review from a team as a code owner November 18, 2024 18:39
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@Cryoris Cryoris added the Changelog: None Do not include in changelog label Nov 18, 2024
ElePT
ElePT previously approved these changes Nov 19, 2024
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Yesss, been wanting to do it for a long time. LGTM

Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

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

I like this change from a user perspective.

I think we should make sure that the signature / documented use of deprecate_func lines up with the new expected usage.

Looks like the tests need updating to look for the new string.

@@ -26,7 +26,7 @@ def deprecate_func(
since: str,
additional_msg: str | None = None,
pending: bool = False,
package_name: str = "qiskit",
package_name: str = "Qiskit",
Copy link
Contributor

@kevinhartman kevinhartman Nov 19, 2024

Choose a reason for hiding this comment

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

The docstring for this argument indicates that it ought to be the PyPI package name. Assuming we want to make this change, we ought to update the docstring (and probably the arg name as well, e.g. package_description).

Do we use deprecate_func for packages other than Qiskit at this point? If not, perhaps we should just remove the argument altogether and hard-code "Qiskit" within.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I stumbled over this too... I'd prefer not to change the argument name for backwards compatibility (and avoid using @deprecate_arg on this 😂), but we should change the description. Maybe

package_name: The name shown in the deprecation message (use e.g. the PyPI package name).

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 5dd8262, along with a test that setting package_name works as expected

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11917311198

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage remained the same at 88.944%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 92.98%
qiskit/transpiler/passes/optimization/consolidate_blocks.py 2 96.36%
crates/qasm2/src/parse.rs 6 97.15%
Totals Coverage Status
Change from base Build 11842392021: 0.0%
Covered Lines: 79411
Relevant Lines: 89282

💛 - Coveralls

@1ucian0
Copy link
Member

1ucian0 commented Nov 20, 2024

Super minor comment: could it be 1.3 instead of 1.3.0?

@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 26, 2024

@1ucian0 whether it's 1.3 or 1.3.0 is set by the caller, we're not setting a default here 🙂 some other parts in the code also explicitly with 1.3.0 so I'd prefer not changing it, if it's fine by you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants