#Defensive programming

1 messages · Page 1 of 1 (latest)

tight wyvern
#

So what I notice sometimes is that there are PRs where there is a lot of isinstance checks. Now these are not bad, but sometimes I feel like they are used just to cover the "what if x happens", which would probably never

#

There are more cases in there, but in this case I think a good example can be

# 
        try:
            devices = await self.api.async_get_devices()
        except FlussApiClientAuthenticationError as err:
            raise ConfigEntryError(f"Authentication failed: {err}") from err
        except FlussApiClientError as err:
            raise UpdateFailed(f"Error fetching Fluss devices: {err}") from err

        if not isinstance(devices, dict):
            raise UpdateFailed("Error fetching Fluss devices: invalid response data")

        raw_device_list = devices.get("devices")
        if not isinstance(raw_device_list, list):
            raise UpdateFailed("Error fetching Fluss devices: invalid devices data") 

        device_list: list[dict[str, Any]] = [
            device
            for device in raw_device_list
            if isinstance(device, dict)
            and isinstance(device.get("userPermissions"), dict)
            and bool(device["userPermissions"].get("canUseWiFi")) 
            and isinstance(device.get("deviceId"), str)
            and bool(device["deviceId"])
        ]
errant nacelle
#

Yea, they shouldn't be thrown in just to be safe - it kind of depends on the library wheter or not it's needed.

tight wyvern
#

The self.api.async_get_devices() method has a response.raise_for_status(), so whenever the API returns with a non-positive response, it will raise anyway and that will be caught

#

So the only way this could fail is if the API returns a 200, with an invalid body

#

Can that happen? I guess so, but do we expect the integration to work at all then? Not really

#

So I think it'd be a nice addition to our AI instructions to make sure we don't see this when we don't need it

errant nacelle
#

does copilot have access to the libraries during reviews? Otherwise it's kind of hard to define when it is needed.