tronikos / opower

A Python library for getting historical and forecasted usage/cost from utilities that use opower.com such as PG&E
Apache License 2.0
54 stars 49 forks source link

Evergy uses a dynamic opower.com subdomain #10

Closed mark-ignacio closed 11 months ago

mark-ignacio commented 11 months ago

It looks like Evergy is the odd one out, as it's the only utility in this library that requires an API call to determine whether to use kcpk.opower.com or kcpl.opower.com.

I opened this issue because it looks like this might require making greater changes in order to make UtilityBase.subdomain() to be a cached call that doesn't hit a non-opower API every time, and I don't really want to make larger changes than #9 without talking about it first. šŸ˜…

To me it appears that the general, overarching issue is that UtilityBase subclasses aren't structurally meant to be stateful and aren't instantiated. They're currently instead used as a sort of function collection registry, which bars the ability for UtilityBase.subdomain() to return different values depending on what account is connected.

https://github.com/tronikos/opower/blob/8c07c33632dd24eb70edcaa9a04ebd1fd39c898a/src/opower/opower.py#L122-L128

The way I'd personally prefer to make Evergy work is for the Opower class to instantiate the UtilityBase subclass it gets here, which would allow the subclass to handle any statefulness itself, should it choose to do so: https://github.com/tronikos/opower/blob/8c07c33632dd24eb70edcaa9a04ebd1fd39c898a/src/opower/opower.py#L142

This would prove handy because self.utility.subdomain() calls could be implemented as normal methods instead of static methods without changing any of its callsites like so: https://github.com/tronikos/opower/blob/8c07c33632dd24eb70edcaa9a04ebd1fd39c898a/src/opower/opower.py#L170-L172

The idea with this implementation is that Evergy.subdomain() would be able to returned a cached value whenever anything calls Evergy.async_login().

The second required change is to _select_utility, hopefully without having to change every other utility class. I'd prefer to create a classmethod called UtilityBase.subdomains() -> set[str], where the default implementation is just return cls.subdomain(). This allows the Evergy class to be addressed by both kcpk or kcpl with a minor code change to _select_utility to use that instead.

Definitely open to feedback and any suggestions on how you'd like this to be implemented. Even if you'd prefer to make the change yourself!

tronikos commented 11 months ago

Luckily line 126 is unused so I just removed it. This allows us to defer any refactoring for now. I made a small change in Evergy.py and added a TODO for you.