Return detailed forecast values in implied metric units

[lymanepp]

I reworked my previous PR to standardize detailed forecast values as implied metric values

[MatthewFlamm]

I like having it cleaner this way, thanks! There are several other changes too so I need to review this next.

I didn't fully appreciate that the detailed forecast was also in metric units. It is awkward that the default values for the simple hourly and daily forecasts are in imperial units.

[MatthewFlamm]

Looking good to me. Minor comment. Should we be testing the code with mypy, in a separate PR, now that you've been adding typing? Thanks for this BTW.

[MatthewFlamm]

I've just learned that the feature flags:

"Feature-Flags: forecast_temperature_qv,forecast_wind_speed_qv"

will make the simple forecasts return the temperature and wind speed in a consistent SI format as the observations and detailed forecast. The textual forecast will remain in imperial (unless you also ask for si units)

[lymanepp]

I've just learned that feature flags will make the simple forecasts return the temperature and wind speed in a consistent SI format as the observations and detailed forecast. The textual forecast will remain in imperial (unless you also ask for si units)

Since the NWS only covers the US, most pynws users will likely be in the US. So using imperial units for everything might make sense. However, that is a significant breaking change.

Having a default unit system setting on the Nws base class would be less impactful. It could be set to metric, imperial or unset (which would retain existing behavior).

Thoughts?

[lymanepp]

Looking good to me. Minor comment. Should we be testing the code with mypy, in a separate PR, now that you've been adding typing? Thanks for this BTW.

I've been using pylint which is already in the pynws checkin actions, but I've just been dabbling. I'll take a look at mypy when I have some time.

[MatthewFlamm]

Since the NWS only covers the US, most pynws users will likely be in the US. So using imperial units for everything might make sense. However, that is a significant breaking change.

The feature flags I'm talking about not only give you SI units, but gives the same structure, i.e a dict with value and unit. This alignment is also very useful. I'm not sure how "in production" these features are, so it is probably best to just keep watching it. It isn't worth having a breaking change for a feature that isn't promised to be in the API.

Having a default unit system setting on the Nws base class would be less impactful. It could be set to metric, imperial or unset (which would retain existing behavior).

I agree. This needs more continued thought to get right. Trying to aim for consistency first is a good first goal, which this PR provides.

[MatthewFlamm]

Can we type value and the return? I think we need to use this https://stackoverflow.com/a/69383462/11423283

[lymanepp]

Can we type value and the return? I think we need to use this https://stackoverflow.com/a/69383462/11423283

Fixed

[lymanepp]

I'll take a look at mypy when I have some time.

I played with mypy a few minutes last night and found lots of issues. I'll fix the ones caused by this PR.

[lymanepp]

Python 3.7 and 3.8 support for type hinting is limited. The current build errors are due to this. I'm not sure that there's a compatible syntax that will work for 3.7 or 3.8. If not, we'll either need to revert the type hints or change the minimum version to 3.9.

______________________ ERROR collecting tests/test_nws.py ______________________
tests/test_nws.py:2: in <module>
    from pynws import Nws, NwsError, DetailedForecast
pynws/__init__.py:7: in <module>
    from .nws import *
pynws/nws.py:12: in <module>
    from .forecast import DetailedForecast
pynws/forecast.py:25: in <module>
    _TimeValues = list[tuple[datetime, datetime, DetailValue]]
E   TypeError: 'type' object is not subscriptable

[MatthewFlamm]

Python 3.7 and 3.8 support for type hinting is limited. The current build errors are due to this. I'm not sure that there's a compatible syntax that will work for 3.7 or 3.8. If not, we'll either need to revert the type hints or change the minimum version to 3.9.

Have you tried typing.List?

[lymanepp]

Have you tried typing.List?

That works. Thanks!