-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adding bindings for the "Patents" API #13
Conversation
@jeorryb Okay so my first question would be, which component are you interested in focusing on? In this contribution, you have some command-line parsing, as well as a function to pull patent data. We want to be very clear about what each module does, and these two things will definitely be in different modules. |
I can focus on either one. I can tackle the individual functions that each nasa api calls upon. Is it safe to say for now if I just pull out the function that calls on the individual url using the requests module and use that for patent.py we would be in a good spot? In the meantime I'll take a look at #11 as well |
@jeorryb My recommendation is that you just focus on the Patents API for now. If you were to be writing a Python script, and there was a library out there that allowed access to this API, how would it look like if you LOVED working with it? Design your part of the library that way. In particular, focus on code re-use, and deterministic input/output (remember, we have to write unit tests for these!) I'll leave more specific comments inline as part of the PR review. |
return pretty_print | ||
|
||
|
||
def main(): |
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 is a good idea, but not the right place for this. This file won't be run directly by the user, but by other Python modules, so anything related to arguments should go elsewhere. I'd remove this function, and everything below it, as this module needs to focus exclusively on functions for the Patents API.
|
||
if 'error' in body: | ||
raise Exception(body['error']) | ||
# Current default is to print out json, need to add ability for file output |
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.
Remember, these functions will be consumed by other Python modules. We really aren't looking to interact with the user in any way here, but to return useful data that other Python code can consume. How about a dictionary?
|
||
|
||
def patent(**kwargs): | ||
patent_url = 'https://api.nasa.gov/patents/content' |
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 would remove this static URL and instead make it a parameter to this function. We'll want to provide the URL for this API via a configuration file, most likely. We generally want to stay away from statically declaring things in a function.
import json | ||
|
||
|
||
def patent(**kwargs): |
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.
The function name should indicate what the function does, not what it is. I would recommend you create a class called Patents
, and have this function inside (rename the function to get
or something like that)
Also, I cheated and read ahead a little bit, and you likely don't need to capture **kwargs here. Try to limit your input to what you'll actually use. See a future comment for more info here.
|
||
def patent(**kwargs): | ||
patent_url = 'https://api.nasa.gov/patents/content' | ||
get_params = {} |
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 am not understanding this and the next two lines. You essentially create a dict get_params
then add all of the key/value pairs from kwargs
, making it identical. Then you never use get_params
. Was there a reason for this?
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.
That is me not checking my work. I left kwargs in the get_response line to validate it would work and when it did I didn't go back and remove the get_params dict.
get_params = {} | ||
for key, value in kwargs.iteritems(): | ||
get_params[key] = value | ||
get_response = requests.get(patent_url, params = kwargs) |
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.
Kind of what I was talking about before - there doesn't seem to be a reason to pass kwargs in here - can you elaborate on why you did this?
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.
See my previous comment about not removing the get_params dict after testing kwargs.
get_params[key] = value | ||
get_response = requests.get(patent_url, params = kwargs) | ||
|
||
if get_response.status_code == 429: |
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.
Great idea, but let's make sure we capture other error codes. A common approach is to throw Exceptions for anything above 2XX, but I'll leave that up to you.
get_params = {} | ||
for key, value in kwargs.iteritems(): | ||
get_params[key] = value | ||
get_response = requests.get(patent_url, params = kwargs) |
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.
Also, get_response
kind of sounds like a function name. How about just response
?
|
||
body = get_response.json() | ||
|
||
if 'error' in body: |
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.
Is this how the Patents API reports errors? Genuinely curious, I haven't poked around it yet.
Merge remote-tracking branch 'upstream/master'
Technically this belongs in the API binding section. I was planning on basing the rest of the api bindings on the one I used to create patent.py. Right now it just prints results to the screen. I'm planning on adding an argument that allows output to a file.