Issue 1799

[aleferna12]

Please complete the following checklist:

• I have read the guidelines in CONTRIBUTING.md.

• I have documented all public-facing changes in CHANGELOG.md.

• This pull request includes code, documentation, or other content derived from external source(s). If this is the case, ensure the external source's license is compatible with scikit-bio's license. Include the license in the licenses directory and add a comment in the code giving proper attribution. Ensure any other requirements set forth by the license and/or author are satisfied. It is your responsibility to disclose code, documentation, or other content derived from external source(s). If you have questions about whether something can be included in the project or how to give proper attribution, include those questions in your pull request and a reviewer will assist you.

• This pull request does not include code, documentation, or other content derived from external source(s).

Note: REVIEWING.md may also be helpful to see some of the things code reviewers will be verifying when reviewing your pull request.

[Spill-Tea]

The biggest Problem I have with this pull request, is that it is effectively copying Biopython's implementation. The only change here is to explicitly define available parameters instead of the simpler kwargs, with perhaps inclusion of more error messages.

I am also not a fan of global variables controlling the api_key for this module. It seems more appropriate for the api_key to be an instance attribute here.

[aleferna12]

Well it really isn't a copy of anything, although it does take some inspiration from biopython's implementation of course. I prototyped a bunch of different implementarions at the time and this was the one that felt simpler to use and more in line with skbio's API, which to me is an indication that it simply happens to be the best way to implement these features. It would be no good doing things differently just for the sake of doing them differently. This implementation is also a big improvement over biopython's implementation in terms of reliability (the error messages you mentioned, while biopython usually fails silently), convenience (converting the resulting querry to skbio objects such as alignments etc instead of returning streams to be parsed) and documentation. Anyone that has used biopython for entrez querying knows how horribly awkward it feels as a consequence of the issues i mentioned (the chosen output format for queries is especially clunky).

Im not opposed to encapsulating the functions inside a EntrezQuery class to have the API key as an instance attr if you think this is a supperior option. I personally agree that module variables are a bit effy, although to me in this case they are doing more good than bad by avoiding clutter and keeping the API more functional. Another option would be adding the API key as an argument for all functions.

Cheers