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

Make lsqecc pep 561 compliant #260

Merged
merged 2 commits into from
Feb 17, 2022
Merged

Make lsqecc pep 561 compliant #260

merged 2 commits into from
Feb 17, 2022

Conversation

mstechly
Copy link
Contributor

Imagine some other projects wants to use lsqecc. They can use it, but unfortunately the won't be able to run mypy against any imports from lsqecc, unless package is PEP 561 compliant.

This PR adds the components necessary to make it works.

It also solves a couple of other issues:

  1. Due to the configuration in setup.cfg, when I was building lsqecc locally and trying to install with wheel, I noticed that the package is empty. Changing to packages = find: in setup.cfg solved this issue.
  2. I've also added zip_safe=False, as it's known to interfere with editable installations (see here
  3. namespace_packages=true – this will allow to avoid __init__.py at all the levels. I'm not sure if this change is actually desirable, please let me know.
  4. explicit_package_bases=true allows to avoid some issues with mypy configuration in the future.

If you want I can add inline comments to those config files explaining the reason behind them.

@codecov
Copy link

codecov bot commented Feb 16, 2022

Codecov Report

Merging #260 (9a3acb1) into dev (f70a03e) will increase coverage by 0.27%.
The diff coverage is n/a.

❗ Current head 9a3acb1 differs from pull request most recent head b67dd74. Consider uploading reports for the commit b67dd74 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #260      +/-   ##
==========================================
+ Coverage   68.32%   68.60%   +0.27%     
==========================================
  Files          23       23              
  Lines        1926     1978      +52     
==========================================
+ Hits         1316     1357      +41     
- Misses        610      621      +11     
Impacted Files Coverage Δ
...tions/pi_over_2_to_the_n_rz_gate_approximations.py 100.00% <0.00%> (ø)
src/lsqecc/pauli_rotations/rotation.py 97.19% <0.00%> (+0.05%) ⬆️
src/lsqecc/pauli_rotations/circuit.py 72.26% <0.00%> (+0.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f70a03e...b67dd74. Read the comment docs.

Copy link
Member

@gwwatkin gwwatkin left a comment

Choose a reason for hiding this comment

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

Thanks @mstechly. I think all these changes are desirable if they make it easier for other people to use the package. For the __init__.py thing, I think that since all our sources are in the src directory, everything that is not a package will be out of it, which sounds to me like we don't init files at all levels.

@alexnguyenn might have more insight than I do into packaging though.

@mstechly mstechly changed the title Make lsqecc pep 561 complaint Make lsqecc pep 561 compliant Feb 16, 2022
Copy link
Member

@alexnguyenn alexnguyenn left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I agree that these changes are desirable.

  • For namespace_packages=true, I don't think we are using any namespace packages atm, but I am ok with having it there just in case (as long as there is no conflict).
  • It will be great if you could put some comments in as well.

@mstechly
Copy link
Contributor Author

@alexnguyenn I have added some comments and removed some of the options after consideration. It should work fine and I guess it's better to add them when we need them, rather than get into trouble because of having them and not understanding all the consequences.

@alexnguyenn alexnguyenn merged commit f058d8c into latticesurgery-com:dev Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants