-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix pytests for openlibrary/records again #3708
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
Fix pytests for openlibrary/records again #3708
Conversation
"""Converts list of things into the output expected by users of the search API. | ||
|
||
If input_query is non empty, narrow return keys to the ones in | ||
this dictionary. Also, if the keys list is empty, use this to | ||
construct a response with key = None. | ||
""" | ||
input_query = input_query or {} # Avoid a mutable default argument |
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.
thank you
ret = web.ctx.site.save_many(things, 'Import new records.') | ||
return [x.key for x in ret] | ||
web.ctx.site.save_many(things, 'Import new records.') | ||
return [thing['key'] for thing in things] |
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 fine, leaving a note/question re: whether we can guarantee 'key' is indeed a key, i.e. thing.get('key') v. thing['key'] but I imagine if thing
is not None, it should have key
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
Closes #3586 and #3706 Enables
pytest -vv openlibrary/records
and uncovers:json.dumps()
to compare dicts across both Python 2 and Python 3Technical
Testing
Yes. In Docker on both Python 2 and Python 3
Screenshot
Stakeholders