New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return date as datetime instead of string #48
Conversation
Is it possible to detect version and conditionally change return type? |
we would have to introduce a new dependency to do this properly: https://pypi.org/project/awesomeversion/ this is already a core dependency so I don't see this as an issue but let me know if you feel otherwise |
Sure let’s add it! |
To avoid another dependency, couldn't if self.entity_description.key == "telemetry.system.boottime":
if MAJOR_VERSION >= 2022 or (MAJOR_VERSION == 2021 and MINOR_VERSION == 12):
value = utc_from_timestamp(value)
else:
value = utc_from_timestamp(value).isoformat() |
yes but that requires us to do manual version parsing which could break in the future if HA changes versioning schems again, hence the proposed dependency which is designed to handle HA versioning and therefore wouldn't break in the future (in theory). As I mentioned, this is a core dependency, as in it is used in the core of HA, so I think it's a safe one |
I don't fully understand: wouldn't the code below break if schema changes? you are comparing with a specific format of the minimum version, and the schema could change in the future, like you said. Unless awesomeversion does some AI obviously. :) if AwesomeVersion(__version__) < AwesomeVersion("2021.12.0b0"): Thanks for clearing this out for me... |
@raman325 another question, you put this in "homeassistant": "2021.12.0b0", that specifies the minimum version required to run the component is Thanks again for the clarification... |
I suppose that's fair... I could go either way, what would you prefer @travisghansen ?
That was in my initial commit but since travis asked for backwards compatibility, I have removed this. |
If we do version comparison we should do it sanely using the package. I could go either way about adding it and supporting earlier versions though. |
Travis, up to you, I really don't see the need to add another dependency, since we have data internally that tells us the specific version numbers. I also submitted a PR for hass-opnsense with the way I proposed. Your call...:) |
I personally don't see a need for backwards compatibility. Anyone who already has the integration installed just won't be able to update it until they upgrade HA. For new users of the integration, I think it's fair to ask them to be on the latest. The one thing I learned when doing this in another repo is that HACS doesn't tie integration versions to the minimum HA version, it's an all or nothing thing. So once we add a minimum version, it will not be possible for users of older HA versions to install the package, even an earlier compatible one |
I don't agree at all. Using this component does not require HA 2021.12. And I don't think it's fair to force users to upgrade when there's no need to.
But why should a mandatory minimum version requirement be added? I really don't understand. You can use the check in the code we discussed to maintain backwards-compatibility. |
I am just stating the impact of the decision and I don't have a strong preference one way or another. I spoke to travis on discord and he suggested we move forward with updating the minimum version, so I reverted my last commit. |
OK, this was discussed further on discord and the end result is the whatever minimum version that is in the Having said that, the approach I'd like to take is to add version checking in the code and keep the minimum version requirement out (or set it to 2021.10) and just let it sit mostly. The intent is not to support every version in the long-term, but rather to at least give a sane upgrade path without the need to coordinate everything. With that, let's go back to adding the dep (for robust version checking functionality) and let's just set the minimum version in Thanks everyone for the input and help! |
In 2021.12.0 HomeAssistant expects datetime class sensors to return date/time objects instead of a string. You can wait to merge this until the day HA is released if you want but I added a minimum version in
hacs.json
because this would break things on older versions