Descover features from desc.xml #30
Conversation
|
|
I like this too, but as @wuub says we need some fallback. Also, the rxv.py file is growing out of hands. My suggestion is to extract the parsing and support lookup to its own class instead. This way we could have two implementation: one parsing desc.xml and a fallback that returns support for "basic" functions (both with the same interface, of corse). If the first one fails we just switch to the fallback and rxv.py will be agnostic to this. In the long run we could also add special implementions if some receiver is misbehaving, etc. when needed. |
|
Ok, this is a version of this which is in the fail hard camp, but tries to be more explicit about why it's failing with the logging messages. If we wanted to go the other way (fallback if not available), I'll I'd do is not raise in those try blocks, and instead set self._desc_xml to None, then just make all the feature query just check is None before proceeding (and return True unconditionally). |
|
I'm for fail hard right now. But just wanted to note that if it starts causing problems for end-users we might be forced to backtrack a bit. |
desc.xml is fetched during object creation, and we use that to ask the question about wither an Input supports Play_Info instead of hard coded lists. Includes tests.
|
@wuub ok, I'm game for that plan. It probably means a 0.4 bump at least. And at least we have a path to revert to if it turns out to be an issue. I also just added a patch that does a similar thing with the playback_features with some tests. |
|
Sounds like a plan. I still suggest extracting the "support" code to its own class/module to maintain a better design. You could probably also parse the features once and cache the values. It will speed up xml travsersal, decrease memory footprint since you don't have to keep the xml in memory and also make calls using the support methods faster since there's no need to parse xml over and over again. |
This allows us to discover if a given Play method works for a given Source via the features in desc.xml. It's done with some kind of complex xpath, broken into parts, to make it a bit easier to read. Tests include.
|
I think caching the xml is probably a long term optimization. It's so much cheaper than any of the over the network approaches. Parsing the xml is probably at least an order of magnitude or two less than the HTTP calls. I do agree we're getting the point where breaking things up into some sub modules might make sense. But the brain power I have right now isn't quite seeing the best ways to slice things up. We can probably refactor that later once the support is in, and it becomes a little clearer what clumps of functions make sense to split into their own things. |
|
imo refactor should be done in separate PR |
|
@sdague: I believe that we are talking about different things. What I am saying is to just pull the xml once (like you do) and parse everything interesting from it at once and save in for instance a namedtuple or a custom written (value) class. This way we separate the parsing from what is being parsed. Single-responsibility-principle, etc. @wuub: Yeah, I do agree that you should not mix refactoring and new implementation in the same PR. Implementing new features usually trigger refactoring in order to fit the design of the new feature (=refactor so that the new feature can be implemented with a good design). But this doesn't mean that you don't should "use design" when implementing new features. I am so used to work according to TDD and writing a new class (with tests first) to do this parsing is just what I would do. Sorry for being so nit-picky, it's not my intention 😊 I've just seen so many python scripts get out of hands at my current work place (8000+ LOC in a single file...) which are just impossible to refactor. Just don't wanna end up there. |
|
@postlund definitely am starting to see how this extracts. The biggest open issue in my head is that this is source based, but we also need to be zone based for things like power (there was that Zone_4 HDMI issue in HomeAssistant that we basically just said list zones to ignore). I think I'd want to figure out what things we need to expose there before we do that. But I still think if we move forward with something like I've got here, it will let consumers like Home Assistant be a bit more robust. It at least plants the flag for moving forward. |
|
@postlund I agree with you :) we just have to remember that the fact that |
|
@sdague: Since I haven't looked at the format I can't give much advice. But since there are no cycles you will be able to represent it as a tree. Maybe return a list of "zone configurations"? @wuub: Yeah, that is true :) But it's still wise to take care. Large files, even though the code in them are simple tend to end up as god classses, knowing way to much in the end thus being complex anyways. Good idea with a separate place to discuss this! |
sdague
changed the title
|
Just added another commit which reworks the playback support off of desc.xml in a way that I think will be easy to convert to HomeAssistant native support easily. But we will need this stack merged and release before I can write that bit. |
|
Tests are failing for known reasons. I need to build more complicated test scenarios because I actually need the input mapping to work correctly. |
|
@sdague i'm unanle to review this properly right now for family reasons, do you want me to merge this as is to unblock further development? |
This moves get_playback_support from static definitions, to dynamic based parsing desc.xml. The PlaybackSupport object is changed from enum to a set of boolean attributes, for making it easier to convert to equivalent structures at higher levels.
|
With this last fix, that gets our testing working again, it would be great if we could merge this (and maybe get a release out there). I've got a HA patch that uses it to set all the source play bits correctly and dynamically. |
| @@ -252,6 +253,19 @@ def zone_controllers(self): | |||
| controllers.append(zone_ctrl) | |||
| return controllers | |||
|
|
|||
| def supports_method(self, source, method): | |||
There was a problem hiding this comment.
We should probably memoize result of this method.
|
0.4.0 released to PyPI |
This is a WIP looking at pulling features out of the desc.xml. Up for comment and approach, not ready for merge.
desc.xml is fetched during object creation, and we use that to ask the
question about wither an Input supports Play_Info instead of hard
coded lists.