Allow for SI units in forcasts

[makushimu]

This allows to request the forecast data to be natively returned with SI units instead of US customary units.
While the lib currently converts the returned value to SI, it can't really convert the textual observations due to its free form text nature. My ultimate goal is to expose this to home assistant.

There is no change to the default values, so there should be no change to existing code.

Still missing some new tests to cover this feature, but I wanted to at least have this out there for initial review.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.09%. Comparing base (11aec2d) to head (620e513).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   93.79%   94.09%   +0.30%     
==========================================
  Files           9        9              
  Lines         645      661      +16     
==========================================
+ Hits          605      622      +17     
+ Misses         40       39       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MatthewFlamm]

Thanks for this PR! I am +1 for getting this in.

NWS is flaky with respect to honoring the requested units. This library provides some double checks and conversions in the SimpleNWS class in case the NWS returns the wrong unit in some cases.

This code below smells incorrect, although maybe not wrong. And, it isn't commented very well on what the purpose is. There is some conversion between temperature units here. It looks like the conversion can go both ways, so I'm not sure that anything needs to be changed. This maybe more of a note to remember for reviewers including me to look into how this PR interacts with that code bit.

pynws/src/pynws/simple_nws.py

Lines 519 to 529 in 20186a8

	for key in ("probabilityOfPrecipitation", "dewpoint", "relativeHumidity"):
	extracted = SimpleNWS.extract_value(forecast_entry, key)
	if isinstance(extracted, tuple):
	value, value_unit = extracted
	if value_unit.endswith("degC") and temp_unit == "F":
	value = round(float(value) * 1.8 + 32, 0)
	elif value_unit.endswith("degF") and temp_unit == "C":
	value = round((float(value) - 32) / 1.8, 0)
	forecast_entry[key] = int(value)
	else:
	forecast_entry[key] = extracted

[MatthewFlamm]

Some comments on my first read through.

[makushimu]

Cool!
I still want to write some tests around this, and at the same time, I can look into those conversions as well.
That being said, the conversion only seems to be triggering if there is a mismatch between what's being returned for those 3 field ("degC","degF" for the "probabilityOfPrecipitation", "dewpoint" and "relativeHumidity" fields) vs the "temperatureUnit" that was returned. So, if I understand this correctly, this code will just convert those 3 fields to whatever the temperatureUnit was returned.

[makushimu]

Thanks for all the prompt reviews!

[makushimu]

Ah yeah, by using a StrEnum, this limits to python 3.11 and greater. I can probably emulate the StrEnum behaviour with a regular enum that will make it work for older versions.

[lymanepp]

Ah yeah, by using a StrEnum, this limits to python 3.11 and greater. I can probably emulate the StrEnum behaviour with a regular enum that will make it work for older versions.

There's a backport in place already. Look at const.py for usage.

[makushimu]

Should be better now. I'm not sure about the test changes that I did, but that felt like the cleanest way to handle the parameters. I'm open to updating it to something better, of course!

[lymanepp]

Should we move this to const.py where other values are stored? Or will we prefer to make them separate in the future?

[lymanepp]

Also, not sure this needs the Nws prefix as that isn't done elsewhere.

[MatthewFlamm]

I agree with moving to const.py.

We have a bit of mixed bag on naming currently. Most constants do not have a Nws prefix, although the error does. In my experience most python packages do not prefix the package, or related, name on variables inside their namespace. I would vote for no prefix to be most consistent with the rest of the package.

[makushimu]

My idea behind the separate file was because this class would be "public" vs the more "private" values that are stored in the const.py, but this makes sense too.

[MatthewFlamm]

I haven't looked at the testing changes, yet but this is looking really close.

[MatthewFlamm]

One small nit, but otherwise LGTM

[MatthewFlamm]

Thank you! LGTM

[MatthewFlamm]

@lymanepp you have requested changes earlier. We can wait to merge in case you have other comments.