-
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
Changes from all commits
282aa50
c528d41
f7dfe44
7bb5e90
6a8de55
7bd55c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
import argparse | ||
import requests | ||
import json | ||
|
||
|
||
def patent(**kwargs): | ||
patent_url = 'https://api.nasa.gov/patents/content' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
get_params = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, |
||
|
||
if get_response.status_code == 429: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
raise Exception('You have exceeded your rate limit') | ||
|
||
body = get_response.json() | ||
|
||
if 'error' in body: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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 commentThe 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? |
||
pretty_print = json.dumps(body, indent=2) | ||
return pretty_print | ||
|
||
|
||
def main(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
ap = argparse.ArgumentParser(description = 'Search NASA patent portfolio') | ||
ap.add_argument('-a', '--api_key', | ||
help = 'Your NASA API key', required=True) | ||
ap.add_argument('-q', '--query', help = 'Text to search on', required=True) | ||
ap.add_argument('-c', '--concept_tags', | ||
help = 'Return ordered dict of concepts from the abstract') | ||
ap.add_argument('-l', '--limit', help = 'Number of patents to return', | ||
type=int) | ||
args = ap.parse_args() | ||
|
||
#need to add option to output json file | ||
print(patent(**vars(args))) | ||
|
||
if __name__ == '__main__': | ||
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.
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 toget
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.