-
Notifications
You must be signed in to change notification settings - Fork 6
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
Bigger EntrezAPI
docstring and __init__
type hints
#31
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.
Looks good in general, just one request to remove the example for "tool".
easy_entrez/api.py
Outdated
email: E-mail address of the E-utility user. | ||
tool: Name of application making the E-utility call, for NCBI internal tracking. | ||
Value must be a string with no internal spaces. | ||
Example values are "easy-entrez" or "biopython". |
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 not want to recommend any specific values; downstream users can build a custom "tool" using this package as a dependency and they should specify their own tool name.
Example values are "easy-entrez" or "biopython". |
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.
Thanks for the comment! From my side, a large part of the reason I made this PR was because I felt tool
was the most vague arg for EntrezAPI
. As a new Entrez user, I had no idea what a "tool", now I understand it's just metadata NCBI uses. The value of tool
can be any string really.
Biopython users get defaulted to using "biopython"
as their tool: https://github.com/biopython/biopython/blob/biopython-181/Bio/Entrez/__init__.py#L141
I think it's important this docstring provide some concrete examples, I will adjust the wording to make it clear they're examples and not recommendations.
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.
Thanks @jamesbraza
email
#28api_key