-
Notifications
You must be signed in to change notification settings - Fork 15
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
build: Add Pipenv for dependency management #12
build: Add Pipenv for dependency management #12
Conversation
I didn't spot this PR before merging #10, but in general am very much in favour of adding better dependency management 🙌 |
@GalenReich Rad. I'll refactor this to account for the latest changes. Probably not until Wednesday tho 🙂 |
72748e8
to
3b8c6b4
Compare
@GalenReich lmk what you think. Edit: I saw last night that other Bellingcat Python projects use Poetry. Would you prefer that? It's no big |
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 looks great! 🙌 Thanks for sorting this - it'll be much more reliable I'm sure! No worries about using pipenv over poetry, we have a mix of projects using each. Just the one open question about installing the dev dependencies.
README.md
Outdated
|
||
```shell | ||
pip3.12 install pipenv # Installs Pipenv | ||
pipenv install # Installs all dependencies |
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.
Should this be pipenv install --dev
? As otherwise black is not installed.
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. Yes, you are correct. Addressed here: 93bc4b1
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 to me!
Summary
This PR adds Pipenv for improved dependency management.
Rationale
Pipfile.lock
file locks dependencies to ensure consistent dependencies across environments, reducing "it works on my machine" issues.[scripts]
section.I would like to make some enhancements to this project (like adding tests - #11), which would require installing
pytest
. I don't want to makepytest
a production dependency. Users who just want to use the script shouldn't have to install it. So I decided a good first step to enhancing EDGAR is to add a nicer dependency manager. Users can still userequirements.txt
for the original workflow. In the future developers can generaterequirements.txt
viapipenv requirements > requirements.txt
.Pipenv is great. Another alternative is Poetry. Please lmk if you would like further explanation/justification for this PR.