-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FD-1745] Add UMLS as a new search API #6
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.
The changes I described in the comments are not really required, but I do think it would make the code more readable and easier to maintain going forward. It could also become a refactor if necessary.
class OntologyAPI: | ||
def __init__(self, base_url, api_id, api_name): | ||
self.base_url = base_url | ||
self.api_id = api_id | ||
self.api_name = api_name | ||
|
||
def get_api_key(self, api_id): | ||
if api_id == "umls": |
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.
You could override this function at each API child class with an appropriate response. If the default response is the exception, you could throw that in the base class and only override the function in the child classes where it makes sense.
from search_dragon.external_apis import OntologyAPI | ||
from search_dragon import logger | ||
|
||
UMLS_API_BASE_URL = "https://uts-ws.nlm.nih.gov/rest/search/current" |
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.
For these UMLS specific variables, you could define them as part of the class itself (so define them immediately after the class definition you have on line #19). Then, when you refer to them, you would refer to them like: UMLSSearchAPI.base_url or whatever. This helps aggregate those details more closely. Or, if they are actually present in every single class, but different, they can be assigned using the base class init() but each class passes the appropriate values. Then, they would be referred to using self.base_url, etc.
1bb7398
to
73cbe1a
Compare
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.
Looks great! Thanks
No description provided.