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

Chore/python3 #220

Merged
merged 19 commits into from
Jul 11, 2019
Merged

Chore/python3 #220

merged 19 commits into from
Jul 11, 2019

Conversation

paulineribeyre
Copy link
Contributor

@paulineribeyre paulineribeyre commented Jun 7, 2019

  • updated dockerfile to use the python 3 base image
  • ran 2to3 for python 2 to 3 syntax updates
  • ran black on everything and added wool

Breaking Changes

  • Python 3 instead of python 2

Dependency updates

  • Use python 3 versions of authutils (4.0.0) and cdisutilstest (1.0.0)
  • Bump indexclient to 1.6.0

@paulineribeyre paulineribeyre force-pushed the chore/python3 branch 2 times, most recently from 656c1de to f9ecc5f Compare June 10, 2019 20:19
@PlanXCyborg
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@vpsx
Copy link
Contributor

vpsx commented Jul 1, 2019

LGTM :D, try removing that python path in the uwsgi.ini, and someone else should look at the Dockerfile and deployment/uwsgi/uwsgi.ini, just for a second set of eyes, because I am not confident :P

@vpsx
Copy link
Contributor

vpsx commented Jul 1, 2019

Also we should look into what codacy is yelling about

@paulineribeyre
Copy link
Contributor Author

Also we should look into what codacy is yelling about

I can fix some of the issues, but codacy is complaining about a lot of things and i think most (all?) of them are unrelated to this PR, they just show up because black changed all the files

@fantix
Copy link
Contributor

fantix commented Jul 9, 2019

One off-topic comment: could you please specify a bit more about how this PR will impact the deployment in the "Deployment changes" section in the PR description? You can remove that section if this PR requires no special head-ups from DevOps during deployment. :gratitude:

Copy link
Contributor

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! A few trivial comments below, not listing all the "dittos" but I think you get it ;)

hashes = flask.request.args.getlist('hash')
hashes = {h:v for h,v in map(lambda x: x.split(':', 1), hashes)}
hashes = flask.request.args.getlist("hash")
hashes = {h: v for h, v in [x.split(":", 1) for x in hashes]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) replacing [] with () might be slightly more efficient

Initialize the SQLAlchemy database driver.
'''
"""
super(SQLAlchemyAliasDriver, self).__init__(conn, **config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) could be simply super().__init__...

hashes = flask.request.args.getlist('hash')
hashes = {h: v for h, v in map(lambda x: x.split(':', 1), hashes)}
hashes = flask.request.args.getlist("hash")
hashes = {h: v for h, v in [x.split(":", 1) for x in hashes]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@paulineribeyre paulineribeyre merged commit 13f6a7a into master Jul 11, 2019
@paulineribeyre paulineribeyre deleted the chore/python3 branch July 11, 2019 15:28
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.

None yet

4 participants