indykoning / PyPi_GrowattServer

MIT License
70 stars 32 forks source link

Bad default arguments #6

Closed geuben closed 3 years ago

geuben commented 3 years ago

https://github.com/indykoning/PyPi_GrowattServer/blob/e14f8d349469d5ba412a88a7733fc226528c8226/growattServer/__init__.py#L266

https://github.com/indykoning/PyPi_GrowattServer/blob/e14f8d349469d5ba412a88a7733fc226528c8226/growattServer/__init__.py#L332

using datetime.datetime.now() as a default argument does not work as you would expect. The default value will be fixed at the time the method is defined (during program startup) and not be the time the method is called.

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

solution would be:

def mix_detail(self, mix_id, plant_id, timespan=Timespan.hour, date=None):
    if date is None:
        date = datetime.datetime.now()
    ...
indykoning commented 3 years ago

Thank you 🚀 @muppet3000 would you be able to take a look and test it? i do not have a mix inverter to test it on.

muppet3000 commented 3 years ago

@indykoning more than happy to take a look and resolve this. While I'm doing it, do you want me to change all of the other functions that take date as an argument to use the same pattern i.e. have a default argument of None that will fall back to the current date if one isn't provided? It's fully backwards compatible and we can move all of the if timespan == Timespan.day.......... into a single helper function and avoid any repeated code.

muppet3000 commented 3 years ago

I've submitted 2 PRs for this, you only need one of them though depending on what approach you want to take @indykoning

This one: https://github.com/indykoning/PyPi_GrowattServer/pull/7 solves the problem mentioned above only and adds no other changes.

This one: https://github.com/indykoning/PyPi_GrowattServer/pull/8 solves the problem above and also refactors all date handling code to take the same approach. If you want to merge this one (in my opinion the better solution) please make sure you test it on the other types of system first.

Both solutions have been tested on my mix/hybrid inverter system.

indykoning commented 3 years ago

I've merged the PR in 1.0.1 :tada: Thank you for the fixes @muppet3000