Fix make_http_request

[matthewlchambers]

Improved control flow and robustness of make_http_request, and renamed it as make_url_request to reflect the fact that it can make FTP requests as well. I also provided make_http_request as an alias for make_url_request, with a logged deprecation warning, since stewi imports and uses make_http_request from this package(I've also submitted a PR on stewi to change the name).

[bl-young]

Will this align with USEPA/flowsa#144, meaning, should flowsa just use the esupy function here?

I haven't yet looked closely but its possible that the original flowsa function existed before esupy so we can probably consolidate there now.

[matthewlchambers]

Will this align with USEPA/flowsa#144, meaning, should flowsa just use the esupy function here?

I haven't yet looked closely but its possible that the original flowsa function existed before esupy so we can probably consolidate there now.

The flowsa function includes a couple of lines to deal with websites that require cookies. It would be easy to add those lines to the esupy function and then have flowsa use the esupy function. Would you like me to make that change and submit a new pull request?

[WesIngwersen]

Will this align with USEPA/flowsa#144, meaning, should flowsa just use the esupy function here?
I haven't yet looked closely but its possible that the original flowsa function existed before esupy so we can probably consolidate there now.

The flowsa function includes a couple of lines to deal with websites that require cookies. It would be easy to add those lines to the esupy function and then have flowsa use the esupy function. Would you like me to make that change and submit a new pull request?

@catherinebirney please weigh in on this

[bl-young]

@matthewlchambers yes I think you can go ahead and make that change here, add that set_cookies parameter the same as used in flowsa than we can drop that function in flowsa and use this one instead

[bl-young]

see comment above

[matthewlchambers]

Ok, I've added the cookie handling code to the esupy function. Before removing the flowsa.common function, do you want me to add the new JobLib related code (that just recently got added to the flowsa.common function as decoration) to the esupy function as well?

[matthewlchambers]

Actually, I was wondering about that JobLib caching anyway; it doesn't seem like we should be calling the exact same url very often. Unless this happens in connection with a data source I'm not familiar with yet?

[bl-young]

Before removing the flowsa.common function, do you want me to add the new JobLib related code (that just recently got added to the flowsa.common function as decoration) to the esupy function as well?

I was just looking to see if we can cache a function call from another package, that is, maintain the JobLib memory caching only for flowsa. I'm not sure we want to always cache these across the tool ecosystem. I suppose in flowsa we could create a pass through function that has the cache.

[matthewlchambers]

Can you point me to the use case that prompted implementing caching? Also, I wonder if we should be using a caching method specifically intended for caching http requests, such as that found in the package requests-cache, rather than JobLib.

[matthewlchambers]

In any case, we can certainly implement a pass-through function in flowsa that caches using JobLib if that's desired.

[bl-young]

Let's discuss further when @catherinebirney returns next week. She was the one who implemented the caching so I don't want to speak for her. If i recall it was suggested by one of the package reviewers

[catherinebirney]

I agree with dropping make_url_request() from flowsa and calling on the esupy function. I've thought about the JobLib caching as well - it is helpful for developers for testing purposes, but outside of testing, JobLib is not overly useful because as @matthewlchambers points out, the same urls are not called often. I'm open to dropping JobLib from the repo and I'll continue to use caching locally

[bl-young]

merging in to esupy, @catherinebirney @matthewlchambers i'll let you all decide on what you'd like to do about the caching in flowsa and what other updates are needed there to transition to using this function.

[matthewlchambers]

@catherinebirney , I'll go ahead then and submit a pull request removing the JobLib caching and replacing the flowsa.common function with the esupy function throughout the flowsa package.