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

Fixes for the API change of construct v2.9.30 #220

Merged
merged 3 commits into from
Feb 13, 2018

Conversation

syssi
Copy link
Collaborator

@syssi syssi commented Feb 13, 2018

Pinning (construct==2.9.30) must be the consequence for the present. As long as construct hasn't a stable api no pinning will be brinkmanship. 😡

Fixes #217.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 68.04% when pulling 635c97e on syssi:feature/fix-for-construct-2.9.30 into bc9fdcf on rytilahti:master.

@coveralls
Copy link

coveralls commented Feb 13, 2018

Coverage Status

Coverage remained the same at 68.04% when pulling 90b5aed on syssi:feature/fix-for-construct-2.9.30 into bc9fdcf on rytilahti:master.

@syssi
Copy link
Collaborator Author

syssi commented Feb 13, 2018

@rytilahti If you ACK this PR I will prepare a bugfix release as soon as possible.

@@ -130,10 +130,10 @@ def play(self, command: str):


class ProntoPulseAdapter(Adapter):
def _decode(self, obj, context):
def _decode(self, obj, context, path):
Copy link
Contributor

@yawor yawor Feb 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code doesn't use the new path arg, wouldn't it be better to add here (and in other methods) *args, **kwargs to just ignore any extra arguments? It would make it backward compatible with earlier Construct versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach! I updated the PR.

@rytilahti
Copy link
Owner

Ok, let's merge this and get a new release out. I'll start looking into Kaitai to see if we can replace construct for good, it is getting out of control if our simple protocol handling gets broken this often.

@rytilahti rytilahti merged commit a675ef0 into rytilahti:master Feb 13, 2018
@arekbulski
Copy link
Contributor

See #222 . I will help you with getting miio uptodate with construct.

@arekbulski
Copy link
Contributor

Using *args **kwargs is not recommended, because if those methods change in the future, it might fail in some silent way. You should undo the 2nd commit.

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.

5 participants