search

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
65 stars 59 forks source link

Fix for AEP Ohio data download home-assistant/core#112744 #72

Closed drewclauson closed 8 months ago

drewclauson commented 8 months ago

Fix for AEP Ohio data download home-assistant/core#112744

drewclauson commented 8 months ago

@tronikos @benhoff I'm open to all feedback, since it's been a long time since I've done anything Python. Thanks!

benhoff commented 8 months ago

Doesn't @abstractmethod force you to implement the method in every subclass, or it will break the subclass and prevent it from being instantiated in the first place, right?

This commit would break the other implementations/subclasses of AEPBase if that is correct.

I'm a bit fuzzy on Abstract Base Class and some of these speciality decorators.

benhoff commented 8 months ago

I'm also for removing the extra if statement under async_login, but you'll see the assertion error from this issue of "async_login not called" unless every subclass of AEPBase has the "cookie to be removed" set or implemented.

Having had to backtrack the above issue before, it's not a very helpful or accurate error code.

I'll try to run this branch tomorrow and confirm it works, code overall looks good to me other than that.

drewclauson commented 8 months ago

Doesn't @abstractmethod force you to implement the method in every subclass, or it will break the subclass and prevent it from being instantiated in the first place, right?

This commit would break the other implementations/subclasses of AEPBase if that is correct.

I'm a bit fuzzy on Abstract Base Class and some of these speciality decorators.

I'm also for removing the extra if statement under async_login, but you'll see the assertion error from this issue of "async_login not called" unless every subclass of AEPBase has the "cookie to be removed" set or implemented.

Having had to backtrack the above issue before, it's not a very helpful or accurate error code.

I'll try to run this branch tomorrow and confirm it works, code overall looks good to me other than that.

I'm more fuzzy than you I think, I was mostly looking for something on how to make the method optional... and pass the pre-commit run! So maybe I read the docs wrong... 😇 I'm working on a change to that method to keep other implementations from failing without implementing that method. The problem I ran into was the linter did not like working with Optional[list[str]] (because I wanted to be all fancy and allow multiple domains to be cleared), but I'll experiment with it more!

I've been so buried in Java/TypeScript lately that the Python syntax right now just looks like Latin to me, so thanks for the feedback, it's helping me learn. :-)

Edit: punctuation / clarifying words

drewclauson commented 8 months ago

I made some tweaks. Theoretically... if we are clearing cookies, then we shouldn't need the extra if statement that was added under the issue of "async_login not called", so I added a check for None on cls.login_cookies_to_clear() before that block and added it back in so it'll continue to work for the other implementations. Take a look!

drewclauson commented 8 months ago

Updated PR at @tronikos request. Makes it much simpler.

benhoff commented 8 months ago

I tested this, and it works for me. Code looks good.

tronikos commented 8 months ago

Released. Please send a PR to bump the version on the Home Assistant side.