-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
remove internal add_control
#13177
remove internal add_control
#13177
Conversation
One or more of the following people are relevant to this code:
|
I don't think we need to do this. It's not in the public interface, and I think changing its name like this risks suggesting that naming is what defines the public interface. If we were going to do it, it'd probably be the module we renamed - the function itself is public within ourselves as a package (evidenced by how it's used in more than one file), and it's the module that's private to users. |
Pull Request Test Coverage Report for Build 13694788019Warning: 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
💛 - Coveralls |
This comment was marked as outdated.
This comment was marked as outdated.
add_control
to _add_control
add_control
d889189
to
52e8811
Compare
I renamed the module (and simplified one of the tests so it does not import the private module). |
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.
LGTM (and I have double-checked that add_control
is not mentioned anywhere else in the Qiskit code). Just a few minor comments about the release notes.
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.
Thanks @jakelishman for improving the release note.
Summary
The internal function
add_control
seems to create confusion among users. Here some examples:I think it can be renamed to mitigate that effect.