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

Add warning message for qw data queries #134

Merged
merged 8 commits into from
Mar 8, 2024

Conversation

ehinman
Copy link
Collaborator

@ehinman ehinman commented Feb 20, 2024

This MR adds warnings to nwis.get_qwdata and nwis.get_info when seriesCatalogOutput=True about the impending lack of updates to the qw service. Please review for accuracy in content and that the location and warning specifications are sufficient within the functions. Thanks!

dataretrieval/nwis.py Outdated Show resolved Hide resolved
@ehinman
Copy link
Collaborator Author

ehinman commented Feb 20, 2024

@thodson-usgs works for me. Changed.

@ehinman
Copy link
Collaborator Author

ehinman commented Feb 20, 2024

After chatting with @lstanish-usgs and @ldecicco-USGS, I think I have a few other messages to edit/add. This is still in draft. I will indicate when it's ready.

@ehinman ehinman marked this pull request as draft February 20, 2024 19:47
@ehinman
Copy link
Collaborator Author

ehinman commented Feb 20, 2024

@thodson-usgs, inspired by the approach taken by @ldecicco-USGS in R dataRetrieval, I'm wondering if nwis.get_info() should spit out warnings in multiple places: (1) A user could type in a 'parameterCd' and if I understand correctly, the sites service leverages the qw service to find sites with data for a given pcode. Is this correct? If so, I'd think we'd want to warn any users who use inputs that are water quality related. (2) A user might set seriesCatalogOutput to True, but the returned data may not contain any qw data. In this step, we could inspect the returned dataset for any data_type_cd == 'qw' and warn if present. What are your thoughts on this? Thanks!

@lstanish-usgs
Copy link
Collaborator

@ehinman thanks for the text update! Just one more date update for the nwis.py function, otherwise this looks good from my end. @thodson-usgs

Copy link
Collaborator

@thodson-usgs thodson-usgs 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

@ehinman
Copy link
Collaborator Author

ehinman commented Feb 26, 2024

Can you provide your input, @thodson-usgs? #134 (comment)

@thodson-usgs
Copy link
Collaborator

It's really the job of the service to provide these sorts of warnings, not the wrapping library (we merely convey them).
At least that's how things are supposed to work :).
In our case, I would create a warning function, as you did in wqp, so that you don't have to duplicate the message content.

@ehinman ehinman marked this pull request as ready for review February 26, 2024 19:18
@ehinman
Copy link
Collaborator Author

ehinman commented Feb 26, 2024

Sounds good @thodson-usgs. I am going to create a separate PR for it, so we can at least have some messaging out there right now.

@ehinman
Copy link
Collaborator Author

ehinman commented Mar 1, 2024

This MR is ready to merge on my end. @thodson-usgs @lstanish-usgs.

@lstanish-usgs
Copy link
Collaborator

@thodson-usgs this looks approved from my end, is it OK to go ahead and merge? Thanks

@thodson-usgs thodson-usgs merged commit 593f420 into DOI-USGS:master Mar 8, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants