Initial ERD Reorganization

[simbaja]

Initial reorganization of ERD packages including development of converters and removal of the erd-related utility files. Separated out from dishwasher support as this is more complex and will need more discussion/review. First step for implementing Issue #4.

[simbaja]

No worries, completely understand. I would recommend taking a look at my fork instead of this request, it's considerably farther along (though I'll look through the comments here and see if they apply there too). I've done the following in the fork:

gekitchensdk:

1. Restructured the project, breaking up into individual classes and getting rid of many individual methods in favor of classes.
2. Implemented/refined support for more devices (fridge heater, cooktops, dishwasher)
3. Pushed string conversions from HA to SDK
4. Made both clients share the same interface
5. Fixed up some of the authentication stuff (reauth, retries, etc)

ge_kitchen HA:

1. Similar restructuring to the sdk
2. Implemented support for the same devices/sensors as SDK
3. Cleaned up issues that could cause exceptions that kill the integration
4. Added secondary retry logic when primary (sdk) fails

I think I got most of your TODO items covered, except I didn't clean up the initial OAuth code logic yet.

Let me know when you have a chance to look and then we can figure out the path forward.

On Tue, Jan 12, 2021 at 9:54 PM Andrew Marks notifications@github.com wrote:

@ajmarks requested changes on this pull request.

Hey, sorry for taking so absurdly long to get to this. Things have been a bit nuts with a new job, but that's no excuse. On the whole this looks pretty good. It's mostly just nitpicking about staticmethods and such.

In gekitchen/erd/converters/abstract.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556224054:

+

+

+

⬇️ Suggested change

-

-

-

---

In gekitchen/erd/converters/primitives.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556224888:

• "erd_encode_bool",

• "erd_decode_string",

• "erd_encode_string",

• "erd_decode_timespan",

• "erd_encode_timespan"

+)

+

+import logging

+from datetime import timedelta

+from typing import Optional

+

+from .abstract import ErdValueConverter

+

+_LOGGER = logging.getLogger(name)

+

+class ErdIntConverter(ErdValueConverter[int]):

These should probably all be decorated @staticmethod since they don't use any member variables

In gekitchen/erd/converters/abstract.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556225588:

+class ErdValueConverter(Generic[T], ABC):

• @abstractmethod

• def erd_encode(self, value: T) -> str:

• pass

• @abstractmethod

• def erd_decode(self, value: str) -> T:

• pass

Is there a reason you chose to use an ABC over a Protocol[T]? I'm not requesting you change it, just curious.

In gekitchen/erd/registry.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556225939:

• ErdCode.UPPER_OVEN_AVAILABLE_COOK_MODES: ErdAvailableCookModeConverter(),

• ErdCode.UPPER_OVEN_COOK_MODE: OvenCookModeConverter(),

• ErdCode.CONVECTION_CONVERSION: ErdBoolConverter(),

• ErdCode.HOUR_12_SHUTOFF_ENABLED: ErdBoolConverter(),

• ErdCode.OVEN_CONFIGURATION: OvenConfigurationConverter(),

• ErdCode.OVEN_MODE_MIN_MAX_TEMP: OvenRangesConverter(),

• Dishwasher

• ErdCode.CYCLE_NAME: ErdStringConverter(),

• ErdCode.PODS_REMAINING_VALUE: ErdIntConverter(),

• ErdCode.TIME_REMAINING: ErdTimeSpanConverter(),

• ErdCode.CYCLE_STATE: ErdCycleStateConverter(),

• ErdCode.OPERATING_MODE: ErdOperatingStateConverter(),

• ErdCode.RINSE_AGENT: ErdRinseAgentConverter(),

+})

⬇️ Suggested change

-})

+})

---

In gekitchen/erd/registry.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556226556:

@@ -0,0 +1,85 @@

+from typing import TypedDict

+

+from .codes import ErdCode

+from .converters import *

+

+class ConverterRegistry(TypedDict):

• key: ErdCode

• value: ErdValueConverter

If all of these methods end up being decorated @staticmethod or @classmethod, you can change this to Type[ErdValueConverter] and get rid of all of the instance creation below. Alternatively, if you use a Protocol instead of an ABC, both instance and classes with static/class methods can be hinted as ErdConverterProto or whatever you call it.

In gekitchen/erd/converters/primitives.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556227343:

• "erd_decode_int",

• "erd_encode_int",

• "erd_decode_signed_byte",

• "erd_encode_signed_byte",

• "erd_decode_bytes",

• "erd_encode_bytes",

• "erd_decode_bool",

• "erd_encode_bool",

• "erd_decode_string",

• "erd_encode_string",

• "erd_decode_timespan",

• "erd_encode_timespan"

Do these get used anywhere outside of this file?

In gekitchen/erd/registry.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556227407:

@@ -0,0 +1,85 @@

+from typing import TypedDict

+

+from .codes import ErdCode

+from .converters import *

+

+class ConverterRegistry(TypedDict):

• key: ErdCode

• value: ErdValueConverter

+_registry = ConverterRegistry({

This should be renamed to not have a leading underscore since it's being imported at https://github.com/ajmarks/gekitchen/pull/7/files#diff-6268e47bd3fc9c879628ce37769b861060990bf078a59827fac63ba6710ff5b2R7

In gekitchen/erd/serializer.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556227962:

• :param erd_code: ErdCode or str, the ERD Code the value of which we want to decode

• :param erd_value: The raw ERD code value, usually a hex string without leading "0x"

• :return: The decoded value.

• """

• if erd_value == '':

• return None

• erd_code = self.translate_erd_code(erd_code)

• if isinstance(erd_code, str):

• return erd_decode_bytes(erd_value)

• try:

• return _registry[erd_code].erd_decode(erd_value)

• except KeyError:

• return erd_decode_int(erd_value)

I know this is something I did, but I'm very on the fence about it. What are your thoughts on leaving it as a hex string if we don't know how to decode? I mostly think that would be a better behavior, but I worry that something somewhere implicitly relies on this fallback.

In gekitchen/ge_appliance.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556228224:

+from .erd import ErdCodeType, ErdSerializer

+

+from .erd import ErdApplianceType, ErdCode

⬇️ Suggested change

-from .erd import ErdCodeType, ErdSerializer

-

-from .erd import ErdApplianceType, ErdCode

+from .erd import ErdApplianceType, ErdCode, ErdCodeType, ErdSerializer

---

In gekitchen/erd/values/fridge.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556229419:

• NA = "FF"

+class FridgeIceBucketStatus(NamedTuple):

⬇️ Suggested change

• NA = "FF"

-class FridgeIceBucketStatus(NamedTuple):

• NA = "FF"

+class FridgeIceBucketStatus(NamedTuple):

---

In gekitchen/erd/serializer.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556229836:

@@ -0,0 +1,80 @@

+import logging

+from datetime import timedelta

+from textwrap import wrap

+from typing import Any, Dict, Optional, Set, Tuple, Union

+

+from .codes import ErdCode

+from .registry import _registry

+from .converters import ErdValueConverter

+from .converters.primitives import *

+

+_LOGGER = logging.getLogger(name)

+

+ErdCodeType = Union[ErdCode, str]

+

+class ErdSerializer:

Is this used anywhere except https://github.com/ajmarks/gekitchen/pull/7/files#diff-f7dcdaf6a62f6adad7ea90ceca0165811c2efa8cc4a5ac395f5e0cc84ec0a204R27 ?

If not, since it doesn't have any instance or class variables, it should probably just be three functions without the class trappings. They could either be directly folded into GeAppliance, left here with thin wrapper methods in GeAppliance, or just invoked directed when needed.

In gekitchen/erd/serializer.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556230033:

• def translate_erd_code(self, erd_code: ErdCodeType) -> ErdCodeType:

• """

⬇️ Suggested change

• def translate_erd_code(self, erd_code: ErdCodeType) -> ErdCodeType:

• """

• @staticmethod

• def translate_erd_code(erd_code: ErdCodeType) -> ErdCodeType:

• """

---

In gekitchen/erd/serializer.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556230127:

• """

• if isinstance(erd_code, ErdCode):

• return erd_code

• try:

• return ErdCode[erd_code]

• except KeyError:

• pass

• try:

• return ErdCode(erd_code.lower())

• except ValueError:

• raise UnknownErdCode(f"Unable to resolve erd_code '{erd_code}'")

• return erd_code

• def decode_erd_value(self, erd_code: ErdCodeType, erd_value: str) -> Any:

⬇️ Suggested change

• def decode_erd_value(self, erd_code: ErdCodeType, erd_value: str) -> Any:

• @staticmethod

• def decode_erd_value(erd_code: ErdCodeType, erd_value: str) -> Any:

---

In gekitchen/erd/serializer.py https://github.com/ajmarks/gekitchen/pull/7#discussion_r556230164:

• :return: The decoded value.

• """

• if erd_value == '':

• return None

• erd_code = self.translate_erd_code(erd_code)

• if isinstance(erd_code, str):

• return erd_decode_bytes(erd_value)

• try:

• return _registry[erd_code].erd_decode(erd_value)

• except KeyError:

• return erd_decode_int(erd_value)

• def encode_erd_value(self, erd_code: ErdCodeType, value: Any) -> str:

⬇️ Suggested change

• def encode_erd_value(self, erd_code: ErdCodeType, value: Any) -> str:

• @staticmethod

• def encode_erd_value(erd_code: ErdCodeType, value: Any) -> str:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ajmarks/gekitchen/pull/7#pullrequestreview-566850862, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOEHFXAH4IQVZY42COAR3O3SZUDPXANCNFSM4VDEAARA .