Skip to content

Conversation

@vinisalazar
Copy link
Contributor

Hi,

while working on the docs for the core interface, I realised that the URL parsing and interfaces methods would have to be directly imported from their respective modules, rather than being accessed from the erddapy module. This seemed a bit unintuitive, so I added public methods directly to the erddapy.__init__ module.

Users can then run:

from erddapy import get_download_url, to_pandas

url = get_download_url(...)
df = to_pandas(url)

# or
import erddapy

url = erddapy.get_download_url(...)
df = erddapy.to_pandas(url)

instead of

from erddapy.core.url import get_download_url
from erddapy.core.interfaces import to_pandas

which seems much harder to do.

Summary of changes:

  • Add public methods to erddapy.__init__

This allows using the URL parsing and interfaces methods directly from the erddapy module,
without having to access the core.url and core.interfaces modules.

  - Add public methods to erddapy.__init__

  This allows using the URL parsing and interfaces methods directly from the erddapy module,
  without having to access the core.url and core.interfaces modules.
@abkfenris
Copy link
Contributor

If we are going to expose them all at one spot, I'd rather expose them at erddapy.core rather than at the top level.

I think it would help clarify to users that refactoring is going on, and allow them to make an explicit choice which world to live in. It also would decrease the amount that is in a single namespace.

import erddapy.core

url = erddapy.core.get_download_url(...)

  - Expose methods in erddapy.core instead of erddapy
  - Code review for ioos#281
vinisalazar added a commit to vinisalazar/erddapy that referenced this pull request Nov 22, 2022
@ocefpaf
Copy link
Member

ocefpaf commented Nov 22, 2022

Yep. I agreed with @abkfenris, putting them all at the root level would be a bit confusing for folks browsing the module. Having them into a .core is probably the best place for them.

@vinisalazar
Copy link
Contributor Author

Done with 6fdb280.

Users can run:

import erddapy

erddapy.core.get_download_url()
erddapy.servers
...

i.e. no need to import erddapy.core.

Copy link
Member

@ocefpaf ocefpaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait until Friday just in case Alex see this before then.

@ocefpaf ocefpaf merged commit ecc621c into ioos:main Nov 23, 2022
@ocefpaf ocefpaf added the GSoC22 label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants