Start refactoring project

[vinisalazar]

Hi,

I have started work on the refactoring project. This PR creates the core subpackage and moves the url_handling and netcdf_handling modules to that. I was thinking of first consolidating the module/directory structure and then moving on to making the code changes.

I was unsure of whether the erddapy module should go inside the core subpackage. From my understanding, we are eventually going to get rid of the ERDDAP class in favour of the new classes in the object layer (or maybe keep it for backwards-compatibility?). Should other functions in that module (e.g. _search_url, parse_dates, etc.) be moved into a separate module of the core subpackage?

The same goes for the server modules (servers.py, multiple_server_search.py and the erddaps.json file). Should those go into core or into a subpackage of their own?

Lastly, should we create a refactor or layer-refactoring branch to merge this PR into?

Please let me know what you think.

Thank you,
Vini

Summary of changes

• Add files to .gitignore
• Create core subpackage
• Move url_handling and netcdf_handling to new subpackage
• Refactor imports

[abkfenris]

I have started work on the refactoring project. This PR creates the core subpackage and moves the url_handling and netcdf_handling modules to that. I was thinking of first consolidating the module/directory structure and then moving on to making the code changes.

I was unsure of whether the erddapy module should go inside the core subpackage. From my understanding, we are eventually going to get rid of the ERDDAP class in favour of the new classes in the object layer (or maybe keep it for backwards-compatibility?). Should other functions in that module (e.g. _search_url, parse_dates, etc.) be moved into a separate module of the core subpackage?

I think we should keep the existing ERDDAP class and the erddapy module at their current locations for backwards compatibility (eventually with some deprecation warnings). Most of the functions and methods in there should become wrappers around various things in core.

The same goes for the server modules (servers.py, multiple_server_search.py and the erddaps.json file). Should those go into core or into a subpackage of their own?

Hmm, good question. I think we'll probably end up with new, more opinionated multiple server search for the object layer, but the result wrangling will probably be shared. Maybe core/search_result.py when those guts get refactored out, with the existing multiple_server_search.py left in place for compatibility. I'll have to ponder more on servers.py and erddaps.json since those are small but combine fetching and wrangling, but are still really helper functions, unless @ocefpaf has some ideas.

Lastly, should we create a refactor or layer-refactoring branch to merge this PR into?

A refactor branch should do the trick.

[vinisalazar]

Thank you for your comments @abkfenris. I noticed that checks passed now after failing on the multiple server search. I will work on #258 along with the refactoring and hopefully that will prevent randomly failed tests in the future.

[ocefpaf]

I'll have to ponder more on servers.py and erddaps.json since those are small but combine fetching and wrangling, but are still really helper functions, unless @ocefpaf has some ideas.

As we discussed in our meeting that is mostly to check the awesome erddap json server list and create the shortcuts to the servers. We will create a package based on the RAs, see #257, and having those into their own sub-module will be useful in the near future.

[vinisalazar]

After force-pushing to fix a line-break, the test failed again, so I'm thinking it's probably best to work on #258 before merging this one (I will mark it as a draft until then).

[vinisalazar]

I'm hoping that the failing check will pass after merging #260.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[vinisalazar]

I added more changes to this branch, but if it's too much, I can reset it and wait for it to be merged before submitting a separate PR.

Summary of changes

• Move doc_helpers.py module to notebooks
• Move helper functions from multiple_server_search.py to url_handling.py (except for _format_results to avoid pandas import in the URL module)
• Create servers directory

[vinisalazar]

There is a certain degree of repetition for an explicit import of the servers dictionary (from erddapy.servers.servers import servers) but it can also be imported from the top-level package: from erddapy import servers.

[vinisalazar]

I just realised that users doing from erddapy.servers import servers (as would be done before merging this) will import the module and not the dictionary. Maybe there should be a warning for that, or should I make the servers dict object available in the servers/__init__.py module (this way that import would still do the same thing)?

[ocefpaf]

or should I make the servers dict object available in the servers/__init__.py module (this way that import would still do the same thing)?

This is probably better.

[vinisalazar]

Would it be better to create the refactor branch to merge this PR into? Or is main ok?

[vinisalazar]

I'm not sure why the test for multiple server search fails (apparently) randomly. Should we replace it with a VCR response?

[ocefpaf]

I'm not sure why the test for multiple server search fails (apparently) randomly. Should we replace it with a VCR response?

I could not make VRCs work for that test. I don't remember why. Maybe it was the pytest parameters? They may work if we split them up.

Would it be better to create the refactor branch to merge this PR into? Or is main ok?

I prefer main to avoid a huge merge from a refactor branch into main later.

[vinisalazar]

I could not make VRCs work for that test. I don't remember why. Maybe it was the pytest parameters? They may work if we split them up.

I'll look into it, but split what up, exactly?

[abkfenris]

I'm guessing with multi-server search VCR was probably having a hard time matching request to response. It might need a custom request matcher