Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interfaces #277

Merged
merged 11 commits into from
Nov 17, 2022
Merged

Interfaces #277

merged 11 commits into from
Nov 17, 2022

Conversation

vinisalazar
Copy link
Contributor

Hi,

this PR creates module core.interfaces, with methods to use URLs to generate third-party datasets. These methods are used in the ERDDAP class.

I rebased the classes branch from #267 with this branch.

If you could pay special attention to review how kwargs are being propagated across methods, that would be great.

Thanks,
Vini

  - Add interfaces.py to process responses into third-party library objects
  - Use existing dataset
  - Pass requests_kwargs to 'to_pandas' method
@vinisalazar
Copy link
Contributor Author

PS. please add the hacktoberfest labels :)

erddapy/erddapy.py Outdated Show resolved Hide resolved
@ocefpaf
Copy link
Member

ocefpaf commented Nov 9, 2022

PS. please add the hacktoberfest labels :)

Sorry! Missed that completely and we are past the deadline!! Please ping me directly next time.

  - Add protocol check to interfaces function
  - Add self.protocol in ERDDAP.to_ncCF method
  - Don't rename netCDF4 Dataset class
  - Change requests_kwargs default to None
  - Raise exception as object
  - Raise ValueError (instead of return)
  - Get rid of iris exception handling block (in favour of
  pinning latest version)
Copy link
Contributor Author

@vinisalazar vinisalazar left a comment

Choose a reason for hiding this comment

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

Hi @ocefpaf @abkfenris, I addressed all comments that we had noted down, so I think this is ready to be reviewed again.

erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
tests/test_erddapy.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Show resolved Hide resolved
erddapy/core/interfaces.py Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
erddapy/core/interfaces.py Outdated Show resolved Hide resolved
vinisalazar added a commit to vinisalazar/erddapy that referenced this pull request Nov 16, 2022
Code review for ioos#277.

Co-authored-by: Filipe <[email protected]>
  - Initialise dict with curly brackets
  - Improve exception handling in function 'to_pandas'
  - Improve error message in function 'to_ncCF'

Co-authored-by: Filipe <[email protected]>
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.

LGTM. @abkfenris any final comments?

Copy link
Contributor

@abkfenris abkfenris left a comment

Choose a reason for hiding this comment

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

Looks good to me

@ocefpaf ocefpaf merged commit 7a508e6 into ioos:main Nov 17, 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