NWIS Client: Possible case sensitivity issue
jarq6c opened this issue · 7 comments
It appears the NWIS IV service is case-insensitive. For example, note the use of startDT and startdt in the two calls below:
This may be an issue for us because I think the current nwis_client validation is case-sensitive. This has caused some issues when using non-string datetime parameters. For example:
client = IVDataService()
# Ex1. This succeeds
obs = client.get(sites="01013500", startdt="2021-09-01T00:00Z")
# Ex2. This fails
obs = client.get(sites="01013500", startdt=pd.Timestamp("2021-09-01T00:00Z"))
# Ex.3 This suceeds
obs = client.get(sites="01013500", startDT=pd.Timestamp("2021-09-01T00:00Z"))I'm hesitant to have the validator transform any arguments unexpectedly. However, it might be a good idea for the date converter/validator to be case-insensitive. So, if a user enters StArTdT as a parameter the key is passed onto NWIS un-altered, but the value is still treated the same way we currently treat startDT.
ping @aaraney
Hmm, so what it sounds like you are suggesting that we should normalize keyword argument keys? I am fine with dropping the case for keyword arguments. This seems like a feature that we would not advertise in effort to implicitly standardize user usage of the software, but I am all for supporting this. Something like this following should work:
#!/usr/bin/env python3
# typing imports
from typing import Callable, TypeVar
T = TypeVar("T")
def standardize_kwargs(fn: Callable[..., T]) -> Callable[..., T]:
def wrapper(*args, **kwargs):
normalized_kwargs = {k.lower(): v for k, v in kwargs.items()}
return fn(*args, **normalized_kwargs)
return wrapper
@standardize_kwargs
def foo(*, a: str):
return a
def it_works():
print(foo(A=2))
if __name__ == "__main__":
it_works()Actually I'm more partial to standardization. In which case, instead of dropping case-sensitivity, we could raise a helpful error message.
So, maybe something like this?
#!/usr/bin/env python3
# typing imports
from typing import Callable, TypeVar
T = TypeVar("T")
def improved_kwargs_error(fn: Callable[..., T]) -> Callable[..., T]:
def wrapper(*args, **kwargs):
fn_kwargs = set(fn.__annotations__)
for k in kwargs.keys():
if k not in fn_kwargs:
error_message = (
f"key word argument, {k}, not in accepted arguments.\n"
f"accepted arguments are: {fn.__annotations__}"
)
return fn(*args, **kwargs)
return wrapperI like it. What do you think?
I like it. What do you think?
I also like it. I think it is more clear how to resolve an issue which is desirable given the intended audience.
A third option that we did not mention is removing **params from the method call and let python handle arguments that are incorrectly named. We could "forward" any applicable keyword arguments from get_raw to get's method signature. For example, the max_sites_per_request from get_raw could be added to get's method signature.
Im also open to a compromise solution that takes the standardization route that you mentioned, however we just check non-params arguments to verify that they have been passed using the correct case. For example s.get(startdt="01-01-2022", ...) would raise a warning and note that in future versions an exception will be raised rather than a warning. Then in future versions, raise an exceptions when this happens. What are your thoughts @jarq6c?