-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Updated requirements of psycopg2 and pillow to new version. #127
base: master
Are you sure you want to change the base?
Conversation
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.
Glad this is now working on the tests- I have submitted several review suggestions to reduce the changeset.
.github/workflows/lint-and-test.yml
Outdated
black . | ||
- name: Test with django | ||
run: | | ||
python manage.py test |
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 file already exists- seems like it was pulled in on merge? can you rebase onto upstream/master and push back to your fork?
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 was hoping to find some way to undo that - I will get around rebasing it.
requirements.txt
Outdated
pytz==2018.9 | ||
requests==2.21.0 | ||
simplejson==3.16.0 | ||
soupsieve==2.0 |
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.
soupsieve and python-dotenv are just recursive dependencies? Is there a reason you're locking them? If the top-level package requirement guarantees compatibility, we shouldn't need to include all the sub-dependencies with locks in the main file. It will make it more difficult to manage dependencies effectively in the future if we try to track the dependencies of the dependencies as well.
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.
soupsieve
is a transitive dependency, but python-dotenv
is not. I added python-dotenv
to the requirements in #121 to make it easier for people to set up their development environments.
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.
seems that belongs in dev_requirements.txt then- not to be built into deployment packages?
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 am gonna move the python-dotenv
to the dev_requirements.txt
. Question being: should that version be locked? I am gonna leave the version unlocked for the time being, but let me know if there is anything else regarding it.
This is to allow compatibility for Python 3.8 for newcomers while retaining support for 3.6 and 3.7
40a7595
to
8462826
Compare
Got done with the rebase and now I am gonna take out the transitive dependency of beautifulsoup4 |
This is to allow compatibility for Python 3.8 for newcomers while retaining support for 3.6 and 3.7.