Fix multiple search bug

ioos / erddapy

Python interface for ERDDAP

https://ioos.github.io/erddapy/

BSD 3-Clause "New" or "Revised" License

77 stars 30 forks source link

Fix multiple search bug #203

Closed ocefpaf closed 3 years ago

ocefpaf commented 3 years ago

The bug was when using the built-in server list, where the namedtuple object was passed instead of the URL string. The rest of the changes are:

[X] Some functions used here belong only to these functionality and should be private.
[X] One of those was in another module and, since it will be private, it belongs in the multiple_server_search.py instead.
[X] Removed the default argument for the query. It should be mandatory and without a default value to avoid confusion.
[x] Added tests to avoid regression.

ocefpaf commented 3 years ago

@callumrollo I forgot to ping you here, sorry! But I do want your feedback on this PR and I'll try to comment out a few points in the code. We can iterate on it again if you have anything you want to change. When you have the time, please take a quick look at my comments.

callumrollo commented 3 years ago

@ocefpaf thanks for tagging me in this. Looks like you've made great improvements to my rather shoddy first attempt at a multiple search! I'll have a look this evening and try to do a proper code review

callumrollo commented 3 years ago

Still not 100 % sure on how the GithHub code review works, but I've had a look through the changes and pulled the most recent commits locally. Looks a lot more elegant with the refactoring. I think it's been merged in already? I don't have any major comments, other than we should think of the best way to pass information on empty resonposes/non-responding servers to the downstream user. Not sure what the best way of doing this is

ocefpaf commented 3 years ago

Still not 100 % sure on how the GithHub code review works

Well, I wasn't the best coder here b/c I pinged you after I merged :grimacing: But it is pretty much that. We read the changes and discuss the implications on maintainability, usability, and hopefully how to remove bugs.

but I've had a look through the changes and pulled the most recent commits locally. Looks a lot more elegant with the refactoring. I think it's been merged in already? I don't have any major comments, other than we should think of the best way to pass information on empty resonposes/non-responding servers to the downstream user. Not sure what the best way of doing this is

We should probably just log/warn on those. Failing is complicated due to the nature of the servers. Otherwise we will end up like Bob's search tool, with a small subset of really "known to work" servers. We can probably add an option to force fail.