Bug in get_speakers_ip
KennethNielsen opened this issue · 6 comments
While writing a unit test for the get_speakers_ip method I discovered that it does not work on my computer. The second line below caused an AttributeError for the attribute "text".
response = requests.get('http://' + self.speaker_ip + ':1400/status/topology')
text = response.text
Maybe the reason is different versions of the requests library, where this attribute is no longer present.
I have written a fix for it in this code branch https://github.com/KennethNielsen/SoCo/commits/fix_get_speakers_ip. This fix replaces the .text with .content attribute the way it is used in all the other methods (commit 3eb8f5c)
While I had my hands in this methid I also made a few suggested changes for it. For these changes we should probably discuss if they should be merged:
Commit e5639e6 changes the method so that it uses the fact that it is xml it gets back, to parse its way directly down to the ZonePlayer tags. This, I think, should remove the need to check the status of the found ip's (to exclude media servers) the way it was done before, and these check have therefore been removed.
Commit b22f2f8 renames the method and the class variable to ..speaker_ips instead of speakers_ip to make it consistent with the SonosDiscovery class (and I think also makes it more correct English, apparently the plural s should be on the most significant of the nouns in a compound noun, which in this case I believe is the ip)
Hm...which version of Requests are you using? (You can find out by typing pip freeze in the project folder's terminal.) text was added in early 2012, so it's probably best to keep it as is since it's going to be there for the foreseeable future.
Commits e5639e6 and b22f2f8 are sensible, please send a pull request. For b22f2f8, you should be fine doing:
response = requests.get('http://{0}:1400/status/topology'
.format(self.speaker_ip))rather than
response = requests.get('http://{0}:1400/status/topology'
''.format(self.speaker_ip))Ahh, so .text is the "new way". I use version '0.8.2', which is what is packaged in Ubuntu 12.04, so that is probably the issue then.
I can see that the text attribute is only used in this one location. Then the question is what to do about it. 12.04 is the last long term support release, so it is going to be used a while longer, and it is nice to be able to use packaged software. I guess we can:
- Change this one instance of .text to .content and then change all of them later when the new version has spread.
- Error check.
- Make a utility function where we either a) Error check b) Use the old way but then we only have to correct it one place or c) checks version and implements the right way. (all these solutions are too complicated solutions in my mind)
What do you think?
Ahh, I didn't realize that the empty string on the second line was redundant. I'll send in the pull request as soon as we agree on the approach.
Regards Kenneth
From looking Requests version 0.8.2 is from end 2011, the current release is 1.1.0
If there is no downside to using .content then that seems the easy fix to ensure compatibility.
Lets not make it too complicated....
Should we somewhere state the minimum version of requests that this will work with?
Cheers Daivd
David, the requirements.txt would be the place to document such things. See http://www.pip-installer.org/en/latest/requirements.html for supported syntax.
Thanks - still learning.....
This issue is no longer relevant. Closing.