sckott / pytaxize

python port of taxize (taxonomy toolbelt) for R
https://sckott.github.io/pytaxize/
MIT License
34 stars 13 forks source link

Refactored requests code throughout the codebase #29

Closed akshayah3 closed 9 years ago

akshayah3 commented 9 years ago

@sckott I have added a function named requests_refactor which refactors the similar requests code throughout the codebase.

panks commented 9 years ago

@akshayah3 You sure this function is generic enough? Here you are assuming that from a requests GET or POST call we would always need either a json return or a raw return which isn't actually true. Like today I implemented resolver for Taxosaurus(https://github.com/sckott/pytaxize/pull/28), you can't use this function there. With Taxosaurus when we first make a call to the API it redirects us to another url, which we need to keep pinging until we get the results there. You get it by reading the .url or the return of requests. If you return json or raw, that information is lost. Yeah we can try to extract that url from the json ouput, in the case of Taxosaurus, but then in future we might have to deal with similar other cases, where we aren't particularly interested in the json or raw return. Maybe we would be better off just abstracting the part where the call is made and return the output as it is, but then again that is just a two lines code.

akshayah3 commented 9 years ago

@panks I got this idea of refactoring the requests code when I looked at _itisGET which is used by almost all the functions in the itis module. So I thought of extending this to the codebase. The main reason for me doing this apart from the refactoring was that this would help in debugging the code as I have experienced when I was testing for the input validation of different functions.

I think we have to agree that currently there is some code duplication(might be a lot in the future as we add more modules/functions) with requests. So I think we have to find a way to refactor the code pertaining to requests.

I just looked at your code. I do agree that in some places we don't need to output the json instead we need the attribute of the requests object which is url in your case but at the same time there are places where we need to return the json in your code.

I think I have an idea to tackle this instead of requests_refactor method why don't we convert this into a class with separate methods which return the json , raw, xml output and also a method which will just return the requests object(In the future if we need any new functionality we can just add a new method to the class) as it is and whose attributes can be further used in the code or for example in your Taxosaurus code.

In the above mentioned approach depending on which method of the class you use there will be no code duplication nor any loss in information. I think thats a win win situation :)

@sckott What do you think?

sckott commented 9 years ago

I am definitely in favor of reducing code duplication.

I think a class is a good idea. if you can redo your function.

@panks Even though the refactor function is not completely general across the entire pkg, that's okay, since its reducing a lot of code duplication in other functions.

akshayah3 commented 9 years ago

@sckott Thanks. I am already working on a class prototype. Will send a commit soon.

akshayah3 commented 9 years ago

@sckott I have added the refactor class. The tests are failing in python 3.4 and seem to be unrelated to the changes I have made as they are passing on my local machine.

sckott commented 9 years ago

@akshayah3 okay, I'll merge and fix anything locally

akshayah3 commented 9 years ago

@sckott Thanks! On an unrelated note, I think we need to fix this function here. Any suggestions on how to fix that?

sckott commented 9 years ago

i think that small fxn was meant to concatenate elements of length > 1, but I can't remember now