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

Add pytest GitHub workflow & convert to src/ layout #19

Merged

Conversation

jordan-gillard
Copy link
Contributor

@jordan-gillard jordan-gillard commented Jun 30, 2024

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 runs pytest 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 the shadowfinder CLI executable. These are more sanity checks to assert the library and CLI are packaged correctly. One cool thing about tox is that it runs tests against the packaged version of shadowfinder. That means you're testing what users get when they install via pip install shadowfinder.

You can see that the workflow runs correctly on this very PR!

@jordan-gillard jordan-gillard force-pushed the add-tests-and-use-src-layout branch from 1630016 to 7cb2b40 Compare June 30, 2024 15:34
@jordan-gillard jordan-gillard force-pushed the add-tests-and-use-src-layout branch from 7cb2b40 to c358fa1 Compare June 30, 2024 23:12
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
Copy link
Contributor Author

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.

Comment on lines +20 to +25
- id: poetry-lock
name: poetry lock
entry: poetry lock --no-update
language: system
types: [yaml]
files: ^pyproject\.toml$
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Collaborator

@GalenReich GalenReich Jul 8, 2024

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?

Copy link
Contributor Author

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

tox.ini Show resolved Hide resolved
@jordan-gillard
Copy link
Contributor Author

@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 Triage role, which would allow me to request reviewers. This role sits between Read and Write in terms of permissiveness. The official docs for GitHub roles are here: 📜 Permissions for each role 📜

(I promise I'm not trying to xz Utils you 😆).

@GalenReich GalenReich self-requested a review July 2, 2024 15:17
Copy link
Collaborator

@GalenReich GalenReich left a 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.

@GalenReich
Copy link
Collaborator

Once again, thanks for the detailed comments - it's super helpful! 🙌

@jordan-gillard jordan-gillard requested a review from GalenReich July 8, 2024 21:38
@GalenReich GalenReich merged commit e5f636b into bellingcat:main Jul 9, 2024
4 checks passed
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.

2 participants