-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add pytest GitHub workflow & convert to src/ layout #19
Add pytest GitHub workflow & convert to src/ layout #19
Conversation
This will ease local development and testing with multiple Python interpreter versions.
Also updates the one test to import ShadowFinder.
1630016
to
7cb2b40
Compare
7cb2b40
to
c358fa1
Compare
I created a new project and forgot to run pre-commit install. Oops
- repo: https://github.com/psf/black | ||
rev: 24.2.0 | ||
|
||
- repo: local |
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.
This instructs pre-commit
to use the local versions of hooks. It guarantees that the version of black
thats run on git commit
matches whats in Poetry's lock file.
- id: poetry-lock | ||
name: poetry lock | ||
entry: poetry lock --no-update | ||
language: system | ||
types: [yaml] | ||
files: ^pyproject\.toml$ |
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.
This hook keeps the poetry.lock
file in sync with the dependencies specified in pyproject.toml
.
from shadowfinder.shadowfinder import ShadowFinder | ||
from shadowfinder import ShadowFinder |
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.
I added ShadowFinder
to shadowfinder/__init__.py
, which allows us to import from the shadowfinder/
module like it was a file. This makes it easier for users to import classes/functions/etc. without having to know which file those entities belongs to.
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.
This is great, I assume this will require updating the import in the notebook too - is that right?
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.
Good eye. Updated the import in ShadowFinderColab.ipynb
in ad92a31
@GalenReich This one is ready for review. I don't have the ability to request reviewers or else I'd tag ya. No rush, I probably wont be able to contribute further until this weekend. If you want (no pressure), you could assign me the (I promise I'm not trying to xz Utils you 😆). |
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.
Looks good! Just that one question about updating the import in the notebook.
Once again, thanks for the detailed comments - it's super helpful! 🙌 |
This PR updates the project to use the recommended src/ layout, which is ideal for libraries like this one to keep us from publishing project files (like README.md, .pre-commit-config.yaml, etc), amid other benefits. Poetry also makes using this layout easy, since it installs the project in editable mode by default. That means the extra step that maintainers would normally do,
pip install -e .
, never needs to be remembered (yay 🎉).This PR also adds the
tox.yaml
GitHub workflow. This runspytest
using Python versions 3.9-3.12 on every PR and merge to the main branch. This means we can make changes and publish to PyPi with confidence that we support all actively maintained Python interpreter versions.Finally, I added a test to create an instance of the
ShadowFinder
class with valid arguments and a test that calls theshadowfinder
CLI executable. These are more sanity checks to assert the library and CLI are packaged correctly. One cool thing abouttox
is that it runs tests against the packaged version ofshadowfinder
. That means you're testing what users get when they install viapip install shadowfinder
.You can see that the workflow runs correctly on this very PR!