-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Upgrade to python 3, flask 2, and other dependencies #84
Conversation
This comment has been minimized.
This comment has been minimized.
requests>=2.23.0 | ||
SQLAlchemy>=1.3.16 | ||
requests>=2.27.1 | ||
SQLAlchemy>=1.3.16,<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.
big API changes in sqlalchemy2, so I decided to pin this here for now until we can be sure that mbdata works with it
58a7032
to
20ec67c
Compare
@@ -12,5 +12,5 @@ | |||
packages=find_packages(), | |||
use_scm_version=True, | |||
setup_requires=['setuptools_scm'], | |||
install_requires=open("requirements.txt").read().split(), | |||
install_requires=open("requirements.txt").read().splitlines(), |
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 fixes some of the issues that we had in requirements file (it was splitting on whitespace, not newline). We could also consider removing comments:
[line for line in open("requirements.txt").read().splitlines() if line and not line.startswith('#')]
Still doesn't remove inline comments. I wonder if a specific requirements parser would be a better idea?
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 fix with a comment in requirements.txt is good enough IMO.
That said, I guess its fine to use a special parser for this or moving this other way around as that answer suggests etc. Maybe open a ticket for later improvement.
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.
of course, perhaps the better solution would be to switch to poetry/pyproject.toml instead, which should be able to reuse the same dependency list...
This comment has been minimized.
This comment has been minimized.
I don't know why that guard exists but if I were to guess, redis-4.0 (redis 4 wasn't released until much later that year but the documentation of redis 3.5.3 specified that 4.0 wouldn't support py2.7) does not support py2.7 but at time of #45 BU still supported py2.7. |
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.
LGTM, Thanks!
But we should probably test at least 1 downstream app before merging and releasing.
20ec67c
to
bcc913c
Compare
I'm not sure why redis had a guard on it to not install versions greater than v4. @amCap1712 do you know why? I didn't add a specific reason in #45