Power and Energy confusion

jgriss / FusionSolarPy

A basic client to the Huawei Fusion Solar cloud interface for solar power plants

MIT License

30 stars 12 forks source link

Power and Energy confusion #15

Closed schmocker closed 9 months ago

schmocker commented 11 months ago

I have solar panels on my roof since last weekend and now I checked out your package. Thank you for this wonderful piece of code.

I'm happy to contribute to that. What I found so far are the confusing variable names:

total_power_today_kwh
total_power_kwh

It's physically wrong to assign the unit kwh to power. What you mean is energy, so we should actually call it "energy" (or "energy yield":

enery_today_kwh
enery_kwh

Coming from the energy engineering I can tell you that this is a very common confusion - but it's like saying a value with unit km/h is a distance - it's just not correct :). So we should avoid it in order to spread further confusion.

I changed the variables names where I could find them. I'll have a deeper look into the package and add commits.

jgriss commented 10 months ago

Hi @schmocker

Sorry for the late reply!

Thanks a lot for the detailed look at the code. You are absolutely right about this - these were mistakes on my side.

There is already (luckily) a bit of code out there using the package. Changing these variable names would break that code. What do you think about adding additional (correctly named) function in addition to the incorrect ones?

mjaschen commented 9 months ago

Hi!

There is already (luckily) a bit of code out there using the package. Changing these variable names would break that code. What do you think about adding additional (correctly named) function in addition to the incorrect ones?

That would be definitely an improvement for now! 👍

IMHO, it would be ok to change the names and document that change with a major release (e.g. 0.x → 1.0). This library is currently in the 0.x version region, which normally means[^1] that the API can change and break.

[^1]: at least in "Semantic Versioning" land, semver.org

schmocker commented 9 months ago

Hi @jgriss @mjaschen!

I see both points of you. So I added the option to use the the deprecated names with kwargs and property functions while warning the users about the deprecated parameters. Should you ever move to 1.x.x I would definitely remove that as @mjaschen mentioned.

jgriss commented 9 months ago

HI @schmocker & @mjaschen

Thanks for your input! Will definitely clean up the code once we reach a (somewhat) stable version.