Construct related, support upto 2.9.31 #226
Conversation
| @@ -184,7 +184,7 @@ def discover(addr: str=None) -> Any: | |||
| if addr[0] not in seen_addrs: | |||
| _LOGGER.info(" IP %s (ID: %s) - token: %s", | |||
| addr[0], | |||
| m.header.value.device_id.decode(), | |||
| binascii.hexlify(m.header.value.device_id).decode(), | |||
| @@ -133,7 +133,7 @@ def do_discover(self) -> Message: | |||
| :raises DeviceException: if the device could not be discovered.""" | |||
| m = Device.discover(self.ip) | |||
| if m is not None: | |||
| self._device_id = m.header.value.device_id | |||
| self._device_id = binascii.hexlify(m.header.value.device_id) | |||
There was a problem hiding this comment.
Should this be left as it is, considering that it gets passed back to the device at later point, or will it be handled automatically by the build(). If that's fine, I think we can merge this and prepare a new release.
There was a problem hiding this comment.
It recreates behevior from before recent Hex update, just that.
| @@ -184,7 +184,7 @@ def discover(addr: str=None) -> Any: | |||
| if addr[0] not in seen_addrs: | |||
| _LOGGER.info(" IP %s (ID: %s) - token: %s", | |||
| addr[0], | |||
| m.header.value.device_id.decode(), | |||
| binascii.hexlify(m.header.value.device_id).decode(), | |||
| @@ -133,7 +133,7 @@ def do_discover(self) -> Message: | |||
| :raises DeviceException: if the device could not be discovered.""" | |||
| m = Device.discover(self.ip) | |||
| if m is not None: | |||
| self._device_id = m.header.value.device_id | |||
| self._device_id = binascii.hexlify(m.header.value.device_id) | |||
|
Was missing import binascii, fixed and amended. |
1 similar comment
|
I would recommend unpinning, and then rolling out updates the same moment construct makes a release. I can prepare patches for you before construct ships. The next .31 release will change some API too but those changes are already in this PR. I voted against. |
|
@rytilahti Does the number of encode/decode parameters must match? If no we can support >=2.9.23,<=2.9.30. If yes we should use 2.9.30 only. I will be able to test later. |
|
@syssi I think it'd be best to allow non-matching parameters, if not at least for a short while. |
|
Guys, please dont leave me out of this conversation. You talk like I wasnt here, so to speak. I dont want to lecture you, but please bear in mind that I am the construct developer, which I suppose makes me the closest thing to an expert on the topic. I wrote the library, and I am telling you here and now, the patch/hack you are suggesting is not safe. If API changes again, for whatever reason including bugfixes and needed infrastructure upgrades, that hack will fail silently with no indication what is wrong. Stick to newest construct version there is, and upgrade your code accordingly. Frankly I feel like I am not wanted here, so I unwatch the project. I was going to offer, or already did, to submit PRs per every new construct version that affects miio but clearly you dont want that. You can watch Construct issues to get advance notice of changes, if you like. |
|
@arekbulski Thanks for your support. I'm happy to have you on board like every contributor! I'm testing the changes currently and will merge this PR later the day. Could commit default values (None) for the path parameter of encode and decode? With the defaults we are construct==2.9.23 compatible and the setup.py doesn't need updated anymore. Please update the upper bound to 2.9.31. |
|
Path cannot be None, it would fail during concatenation. It could be empty string, would make error messages show inaccurate path info but not fail. Update it yourself, you can edit the branch. |
|
I will care about and update the PR. |
|
@arekbulski your help is very much appreciated and I think we all agree you are the one who knows the inner workings of construct and how to use that in practice, too. I'm sorry that I haven't been very responsive in the past times due to other engagements, but I really do appreciate what you have done with the library even if I don't agree how you have been introducing the changes lately into it. The problem with pinning to a specific construct version has already been discussed and is not really feasible as soon as several projects need to do pinning on newest releases. On the other hand it is hard to keep up with breaking API, especially considering that pip goes and installs the newest from pypi if not instructed otherwise. Going further that path, our biggest downstream is likely the homeassistant, which has a biweekly release cycle, meaning that in the worst case there will be some users left with a broken setup for a couple of weeks (or forced to do some manual patching etc. which may cause troubles later along the road). This is then furthermore bolstered by the fact that e.g. hass.io which in turn has a dependency on homeassistant has its own release cycle and so on... We are very thankful for your offer to help (and I will be checking those patches on other projects of mine where construct is used, I promise!) us to keep our code-base working with newer construct versions, however I personally feel that you are not making it very easy for anyone to feel safe to depend on it in the long run, which I find frankly sad as it is a nice piece of software. |
|
Lets continue our conversation about release cycle in this thread, even if already closed. Yeah, the issues you are describing are familiar to me. You cant pin version (because multiple repos) and you cannot keep up with API changes either (no comprehensive automated testsuite, so you dont see breakage until someone posts a bug report after the fact). Thats why I offered to keep sending you PRs (essentially become co-maintainer, which explains to you why I asked for write-access). The attittude that you two guys displayed earlier doesnt help you, but thats a side issue. The larger issue you have, and I hate to pin this on you (forgive the pinning pun), is mostly on your end. This project has essentially no automated (implied comprehensive) test suite. You complain because the product you delivered broke downstream but you dont realize that its you who are responsible for assembling it. You import a lot of modules, every module changes, every modules breaksdown (regressions). People update things, people get sloppy, mistakes happen. What you are doing is just taking parts that were delivered to you by shipping lane (shipping to pypi = releasing, but also by cargo ships, takes a long travel time), you assembly it without testing, and then ship it (by ship) to your clients (downstream, again a pun). They unpackage it, it doesnt work, they complain and file bug reports. You complain that you cant keep up with API changes, but thats not Construct doing it wrong, its just Construct revealing a fatal flaw in yours. Which is release cycle. Consider this: people sometimes come to Construct and file bug reports (lately there is 1+ per week), or sometimes they request features (like enum34 support). I can have those implemented and shipped out TOMORROW. Kaitai on the other hand, has currently 9-12 month cycle. If you ask them to add or change something, however trivial it is, it will take them a YEAR to ship it. If they somehow fuck it up, like if there arent any bug reports on their forum, they will take ANOTHER YEAR to ship followup fixes. Consider second reason: The only way for Construct to become a better framework is to get feedback from its consumers. If I would make a release monthly, noone would come back after that much time to tell me that I didnt quite fix it and requesting further updates. You would not even bother to write to tech support if it took them a month to send a reply. If your release cycle is monthly, there is no difference there. |
|
Timestamp class was added. In YET UNRELEASED version .34 there is another feature, exposing inlined Enums: |
arekbulski
changed the title
|
A month has passed. Please tell me, what is the status of Construct API issue in general? |
|
Sorry for taking my time with this, I prepared an answer before but thought it was a good idea to let it cool down a bit. To answer your question, there has been no new changes on that matter, I have not had time nor interest working on lower level plumbing of this library. So we have mostly kept hoping things do not break down in the meanwhile. I'm going to answer your questions below and I hope we can close this issue for good, I think we both know our stances on this topic so I don't feel that it's worth time of either of us to debate on this further.
We are accepting PRs, but at the moment I don't see no reason for taking more maintainers in, especially as you are not involved in the project per se.
I agree with the notion that we are lacking in tests (and we also welcome any help on that front), and I'm sorry if you feel that we have not approached this issue fairly. That being said, I don't agree that it is just completely our fault to not have been prepared for breaking changes in a parsing library. As much as I like(d) using construct and how it allows one to do parsing&building, had I known about your position on these issues I would probably just have written the protocol parsing manually. To be fair, I still would recommend it for experimenting & proof-of-concepts where it really shines, but with no API stability it is hardly recommendable for anyone shipping for regular users (maybe the compiled variant will change that, but due to our used structures it is not possible for us to convert).
We are doing this voluntarily (just as you are most likely) and it is not feasible for us to test against every available version nor keep pushing updates just like that. Furthermore I have never had such problems with any other lib I have been depending on as recently with construct, which comes out as weird considering that one would expect a parser (& builder) library be stable -- I would bet that most of the times people sketch a parsing functionality they need and leave it as it is, not many will improve it from time to time just for the sake of improvements in the parser libs.
As I mentioned previously elsewhere, we are somewhat tied with the release cycle of homeassistant, it being likely the upstream with most users of this library, therefore we cannot simply do hotfix updates if some of the upstream libs change suddenly with no deprecation warnings.
I applaud your efforts to provide new features, bug fixes etc. on a timely manner, what I don't agree is the way that it is done. Considering its inherent nature of being a bit more low-level (as parsing is in general), I don't see it being unreasonable to expect a stable API. In a similar manner I think it goes without saying that I don't expect (network) protocols, XML parsers nor JSON parsers to change suddenly now and then, forcing to do a series of updates to existing code-bases that have been working for long time periods just fine.
I understand the point you make, but I hope that the improvements could be done without breaking the backwards compatibility / already existing interfaces. I don't see that being too much asked, you don't see an XML parser or JSON parser breaking their APIs just like that either. Even we as a project try to keep our public API stable and add deprecation warnings where feasible -- |
|
Consider these ideas:
What wont work:
|
This brings construct-related code uptodate, but it requires 2.9.31 which will be shipped within a day.
https://pypi.org/project/construct/
The 2 lines referring to "device_id" are fixed but I am not entirely sure about what should be in those fields (bytes or unicode) so please double check that.
Supercedes #223 and fixes #224 .