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

python 3 conversion #36

Merged
merged 3 commits into from
Mar 29, 2021
Merged

python 3 conversion #36

merged 3 commits into from
Mar 29, 2021

Conversation

golnazads
Copy link
Contributor

No description provided.

Copy link
Member

@kelockhart kelockhart left a comment

Choose a reason for hiding this comment

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

Nearly there! The only main comment is that we'd decided (somewhere in Slack) that we don't need to keep this code backwards compatible with Python 2, so you can remove those lines.

config.py Outdated
@@ -32,6 +32,7 @@
ADS_TWO_POINT_OH_MIRROR = 'adsabs.harvard.edu'

SQLALCHEMY_DATABASE_URI = ""
SQLALCHEMY_TRACK_MODIFICATIONS = True
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to enable this tracking? It consumes a lot of overhead. It looks like previously this was set to None by default (https://stackoverflow.com/questions/33738467/how-do-i-know-if-i-can-disable-sqlalchemy-track-modifications), which was equivalent to True. The error can be resolved by setting this to either True, to keep the tracking on, or False.

https://flask-sqlalchemy.palletsprojects.com/en/2.x/config/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

somehow I did not get notifications of these comments!
OK, shall change to False.

coverage==4.5.4
flask-testing==0.8.1
httmock==1.4.0
httpretty==0.8.10; python_version < '3.0'
Copy link
Member

Choose a reason for hiding this comment

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

I think we decided to not make the code backwards compatible with Python 2, so you can remove these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

harbour/app.py Outdated
@@ -3,19 +3,21 @@
Application factory
"""

from future import standard_library
standard_library.install_aliases()
Copy link
Member

Choose a reason for hiding this comment

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

These two lines are only needed for Python 2 backwards-compatible code, so you can remove them.

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 think these were added automatically when I run futurize. So you are saying I have to go and remove it from all other ones too, lol.

@@ -1,3 +1,4 @@
from builtins import object
Copy link
Member

Choose a reason for hiding this comment

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

Also only needed for Python 2-compatible code.

harbour/utils.py Outdated
@@ -19,7 +19,7 @@ def get_post_data(request, types={}):

if types and isinstance(post_data, dict):
for expected_key in types:
if expected_key not in post_data.keys():
if expected_key not in list(post_data.keys()):
Copy link
Member

Choose a reason for hiding this comment

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

The syntax if key in dict.keys() works ok here in Python 3, without the list() - the converter script is really conservative about adding list() everywhere, even when it's not needed.

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 think this is also part of the futurize! OK, thank you.

harbour/views.py Outdated
@@ -2,26 +2,27 @@
"""
Views
"""
from future import standard_library
standard_library.install_aliases()
Copy link
Member

Choose a reason for hiding this comment

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

These two lines can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

@kelockhart kelockhart 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!

@golnazads golnazads merged commit e4ed8b0 into adsabs:master Mar 29, 2021
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

2 participants