Skip to content
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

Minimum supported version of construct specified #196

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Jan 29, 2018

No description provided.

@syssi syssi requested a review from rytilahti January 29, 2018 06:37
@syssi
Copy link
Collaborator Author

syssi commented Jan 29, 2018

I assume we have to force pip to upgrade dependencies.

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage remained the same at 66.963% when pulling 6e48393 on syssi:feature/lowest-supported-version-of-construct into a50c722 on rytilahti:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.963% when pulling 6e48393 on syssi:feature/lowest-supported-version-of-construct into a50c722 on rytilahti:master.

@yawor
Copy link
Contributor

yawor commented Jan 29, 2018

@syssi what about pinning to a specific version? As history shows, Construct can break compatibility with any version change. I think we should specify exact version in dependencies, not just only minimum version.

@syssi
Copy link
Collaborator Author

syssi commented Jan 29, 2018

If we use pinning all home assistant components must use the same version of construct. It will be hard for foreign libraries to introduce a new version and vice versa.

@yawor
Copy link
Contributor

yawor commented Jan 29, 2018

I didn't thought about that. You're right.
But if different HA dependencies use different construct versions then unfortunately there's a good chance that something is going to break anyway. For example, even requiring minimum version would cause other libraries which are still using context in the "old" way to stop working because we force the update.
There's probably no good solution to this problem in software like HA with such large number of dependencies.

@rytilahti
Copy link
Owner

I think we can try to see if we can convince the construct maintainer to maintain a backwards compatible api between minor releases, but I think such a piece as protocol handling library should neither be pinned (as it will at some point cause failures, if enough users start to use the lib) nor change the public API that often.

Maybe this leaves us porting this over another library, there is suitcase, and kaitai struct (which sound neat, as it will allow reusing the protocol definition also in other languages!), but I haven't tried those. One last option would be simply to wire the protocol as it is and do it all manually, I just thought it'd make more sense to have it in a more maintainable format in case the protocol changes.

I think we should just merge this for the time being for the next release, and see how to deal with it at some later point. Comments?

@yawor
Copy link
Contributor

yawor commented Jan 30, 2018

I've read Construct author's comment on the backward compatibility and I don't think that he can be easily convinced.
Regarding switching to a different library, it would be a pity, as Construct is a really good library (except the backward compatibility issue). I don't know if there are better for the task, I've been using Construct for quite some time now (even before I've known about python-miio) and it always did what I needed it to do, so I've had no reason to look for something else.
Maybe it would be possible to create a compatibility layer for Construct, targeting always the latest version but adapt classes when an older version is installed. It could support 2 or 3 previous versions at most.
I agree that for now this should be merged.

@rytilahti
Copy link
Owner

Yeah indeed, I have also used it in multiple projects without real problems until recently and have liked it sofar. I'm not really sure we should start building compatibility layers for a protocol which changes much less often than the library we are using to parse it, that would be just too much effort that could be targeted elsewhere.

@rytilahti rytilahti merged commit 21065f6 into rytilahti:master Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants