Tweak sensor value setter to validate dtype #98
Conversation
Still potential to consolidate the sensor-type attributes. Contributes to: NGC-1062.
There was a problem hiding this comment.
There are a few corner cases that aren't working:
- The
defaultparameter to the Sensor constructor isn't going through the check. - Setting a discrete sensor to an enum of a different Enum class to the intended one doesn't complain.
- Code that sets the value of a Timestamp sensor using a float used to work fine, but now complains. I think we probably want to support that, since in Python land, Timestamp is really just an alias for float. It's only a different type so that (a)
?sensor-listreports the sensor type correctly and (b) the encoding rules for timestamps are stricter (although that's currently not implemented). I think that can probably be handled as a special case: if the sensor type is Timestamp and the value is an instance of numbers.Real, cast it to Timestamp.
Additionally, you should write unit tests to verify that the new code is raising the exception.
As per comments on PR #98, to check the default value and rework logic to account for cases missed in the naive check previously. Contributes to: NGC-1062.
I'd missed it in the previous update. Contributes to: NGC-1062.
Contributes to: NGC-1062.
|
My updates since the initial review certainly aren't meant to look like anything polished. I was just fed up with trying to neatly wrap the logical checks. Similarly, the added unit test is very naive/plain.
|
To ensure it conforms to the ipaddress.{IPv4, IPv6}Address type.
Contributes to: NGC-1062.
|
Re: Unit tests failure on (ubuntu-20.04, 3.12), I was under the impression I need to ensure compatibility with 3.8(.10)
|
Urgh, I've had similar issues with spead2: there is no version of numpy that installs on both 3.8 and 3.12. Request a review so long (to remind me) and I'll think about how best to handle this when I review. It's tempting to say we should only bother testing the numpy stuff on one version, but numpy has also made subtle tweaks to its type system over time (and less subtle in the 1.x -> 2.0 transition) so we probably should test more than one version. |
Noted all around. I figured it had something to do with the version conundrum. Let me know if I should create a technical-debt ticket for that. |
Missed in a previous commit, apologies. Contributes to: NGC-1062.
| @@ -79,6 +79,8 @@ class Address: | |||
| _IPV6_RE = re.compile(r"^\[(?P<host>[^]]+)\](:(?P<port>\d+))?$") | |||
|
|
|||
| def __init__(self, host: _IPAddress, port: Optional[int] = None) -> None: | |||
| if not isinstance(host, typing.get_args(_IPAddress)): | |||
| raise TypeError(f"{host} is not of either {typing.get_args(_IPAddress)}") | |||
There was a problem hiding this comment.
Coveralls is failing and it looks like it's because there is no test coverage for this. Add a unit test that tries to construct an Address from the wrong type.
Specifically to make sure it is of the correct type. Contributes to: NGC-1062.
tests/test_core.py
Outdated
| @pytest.mark.parametrize("value", ["1.2.3.4"]) | ||
| def test_bad_host(self, value) -> None: | ||
| with pytest.raises(TypeError): | ||
| _ = Address(value) |
There was a problem hiding this comment.
You don't need to assign it to anything. See the test above for example.
| _ = Address(value) | |
| Address(value) |
There was a problem hiding this comment.
Man, and I was wondering about doing it the same way as parse_bad.
There was a problem hiding this comment.
Also, I think I'm going to change the 'bad value' passed in here (to 127.0.0.1) to follow suit with the rest of the tests. No need to be unnecessarily different.
Apologies for the unannounced feedback request @ludwigschwardt @KimMcAlpine.
We spotted something a while back where aiokatcp's
Sensor.set_valuedidn't validatethe dtype of the value coming in to update. Bruce has made me quite aware of the ripple
effect a change like this could make (and likely deconstructive interference). Tagging you
both here mainly to
There is still potential to consolidate the sensor-type attributes (
Sensor.stype).Whenever you can, please and thank you.
Contributes to: NGC-1062.