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

Implement and enforce explicit re-export for cirq modules #6722

Merged
merged 25 commits into from
Sep 11, 2024

Conversation

ashiq-firoz
Copy link
Contributor

@ashiq-firoz ashiq-firoz commented Sep 8, 2024

I have updated mypy.ini inside dev_tools and also made modifications in all init.py files to match

from foo import bar as bar

Ref: #6717

Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please, also update the cirq-core/cirq/__init__.py and remove any wip code such as from foo import bar.

Make sure the CI-reported errors are all addressed. You may want to setup a local development environment following https://github.com/quantumlib/Cirq/blob/main/docs/dev/development.md#setting-up-an-environment.

After that you can use check/mypy, check/pylint to run the respective CI tests locally.
Running check/format-incremental --apply would fix code formatting of any changed files.

Thank you for taking this on!

@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
"""Experiments and tools for characterizing quantum operations."""
from foo import bar as bar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove as well as other debugging code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I will work on that

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Sep 10, 2024
@ashiq-firoz
Copy link
Contributor Author

I have made a push now, please review the pull request @pavoljuhas

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.83%. Comparing base (9edb957) to head (7f98fd3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6722      +/-   ##
==========================================
- Coverage   97.83%   97.83%   -0.01%     
==========================================
  Files        1077     1077              
  Lines       92487    92485       -2     
==========================================
- Hits        90485    90483       -2     
  Misses       2002     2002              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavoljuhas
Copy link
Collaborator

@ashiq-firoz - I have updated your PR so that the no_implicit_reexport mypy rule is checked only for the cirq modules and not for imported dependencies. I have also cleaned up unnecessary re-exports of sub-modules, which are automatically added to their parent namespace. PTAL and let me know if you have comments / suggestions.

If this passes the CI, I feel it should be good to go.

@mhucka mhucka self-requested a review September 11, 2024 23:09
Copy link
Member

@mhucka mhucka left a comment

Choose a reason for hiding this comment

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

LGTM.

@pavoljuhas pavoljuhas changed the title Fixed issue #6717 Implement and enforce explicit re-export for cirq modules Sep 11, 2024
@pavoljuhas pavoljuhas enabled auto-merge (squash) September 11, 2024 23:48
@pavoljuhas pavoljuhas merged commit 2adc218 into quantumlib:main Sep 11, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants