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
Python3 #52
Python3 #52
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.
This looks very good! Like for adsabs/graphics_service#95, I have just a couple suggestions:
- I think we can remove some
list(...)
that I believe are automatically added by the porting tools but they are not needed when there is an iteration in place. - Let's make travis run the tests with python 3, otherwise we may introduce python 2 code in the future without realizing. This requires changing .travis.yml (example: https://github.com/adsabs/adsabs-pyingest/blob/master/.travis.yml).
# get metadata for suggestions | ||
meta_dict = get_meta_data(results=paperFreq[:Nsuggestions]) | ||
if "Error"in meta_dict: | ||
return meta_dict | ||
# return results in required format | ||
return [{'bibcode': x, 'score': y, 'title': meta_dict[x]['title'], | ||
'author':meta_dict[x]['author']} for (x, y) in | ||
paperFreq[:Nsuggestions] if x in meta_dict.keys()] | ||
paperFreq[:Nsuggestions] if x in list(meta_dict.keys())] |
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 think this list(...)
(automatically added by the porting tools) might not be needed here.
@@ -72,7 +72,7 @@ def test_config_values(self): | |||
"CITATION_HELPER_SOLR_PATH", | |||
"DISCOVERER_PUBLISH_ENDPOINT", | |||
"DISCOVERER_SELF_PUBLISH"] | |||
missing = [x for x in required if x not in self.app.config.keys()] | |||
missing = [x for x in required if x not in list(self.app.config.keys())] |
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 think this list(...)
(automatically added by the porting tools) might not be needed here.
# Assert each resource is described has the expected_field | ||
[self.assertIn(expected_field, v) for v in r.json.values()] | ||
[self.assertIn(expected_field, v) for v in list(r.json.values())] |
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 think this list(...)
(automatically added by the porting tools) might not be needed here.
# Assert every expected_field has the proper type | ||
[self.assertIsInstance(v[expected_field], _type) | ||
for v in r.json.values()] | ||
for v in list(r.json.values())] |
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 think this list(...)
(automatically added by the porting tools) might not be needed here.
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.
Perfect!
Port of citation_helper_service to Python 3.8 (according to the "Porting To Python 3" Google Doc)