Update the Russound RNET component to use enhanced Russound.py by altersis · Pull Request #9739 · home-assistant/core

altersis · 2017-10-07T22:48:55Z

Description:

Related issue (if applicable): fixes #9604

Related to issue #6 in the russound rnet component.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
No documentation change is necessary

Example entry for `configuration.yaml` (if applicable): No configuration.yaml changes are necessary

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io -- No new functionality added, only existing functionality has been corrected

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example). No new dependencies added.
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py. No new dependencies added.
New files were added to .coveragerc. No new files added.

If the code does not interact with devices: N/A

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

…t reduces network traffic Refer to issue #6 on the Russound project

… reduce network traffic PLease see issue #6 on the russound project

Please refer to issue #6 on the Russound rnet project

homeassistant · 2017-10-07T22:48:57Z

Hi @altersis,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2017-10-07T22:49:05Z

-            self._source = None
-
+            _LOGGER.error("Could not update status for zone %s", self._zone_id)
+


blank line contains whitespace

houndci-bot · 2017-10-07T22:49:05Z

+            #index = self._russ.get_source('1', self._zone_id)
+            index = ret[1]
+            # Possibility exists that user has defined list of all sources.
+            # If a source is set externally that is beyond the defined list then


line too long (80 > 79 characters)

houndci-bot · 2017-10-07T22:49:05Z

+            #self._volume = self._russ.get_volume('1', self._zone_id) / 100.0
+
+            # Returns 0 based index for source.
+            #index = self._russ.get_source('1', self._zone_id)


block comment should start with '# '

houndci-bot · 2017-10-07T22:49:05Z

+                self._state = STATE_ON
+
+            self._volume = ret[2] * 2 / 100.0
+            #self._volume = self._russ.get_volume('1', self._zone_id) / 100.0


block comment should start with '# '

houndci-bot · 2017-10-07T22:49:05Z

+        _LOGGER.debug("ret= %s", ret)
+        if ret is not None:
+            _LOGGER.debug("Updating status for zone %s", self._zone_id)
+            #if self._russ.get_power('1', self._zone_id) == 0:


block comment should start with '# '

altersis

I have changed the code to satisfy Houndbot style observations

pvizeli · 2017-10-13T08:55:55Z

Ping -> Fix travis please

altersis · 2017-10-16T13:49:23Z

Can I please ask for your help? I'm not familiar with Travis, and for that reason I do not understand what needs to be fixed... I'll do it gladly, just need to understand what's the problem... thanks!

altersis · 2017-10-19T13:43:30Z

Hi Pascal, thanks again for your reminder about the pending PR on Hass! I tried contacting you at pvizeli@syshack.ch<mailto:pvizeli@syshack.ch>, but it seems you did not get my email. I’m a bit stuck on this, and was hoping that someone from the community will contact me. Thanks again for that. Could you please help me understand what the problem is? I read and read the error message, but I simply cannot figure out what the issue is. I can see that there are five jobs on the travis check, and one of them failed, the one related to Python 3.4.2. I read somewhere that Hass will be dropping support for Python 3.4 on release 55, which is out now. Do I still need to worry about a Python 3.4.2 issue? If the we have to, no problem, will do it. From the log, I get that the error is here: GLOB sdist-make: /home/travis/build/home-assistant/home-assistant/setup.py requirements create: /home/travis/build/home-assistant/home-assistant/.tox/requirements requirements inst: /home/travis/build/home-assistant/home-assistant/.tox/dist/homeassistant-0.55.0.zip requirements installed: aiohttp==2.2.5,astral==1.4,async-timeout==1.4.0,chardet==3.0.4,homeassistant==0.55.0,Jinja2==2.9.6,MarkupSafe==1.0,multidict==3.2.0,pytz==2017.2,PyYAML==3.12,requests==2.14.2,typing==3.6.2,voluptuous==0.10.5,yarl==0.13.0 requirements runtests: PYTHONHASHSEED='1077194535' requirements runtests: commands[0] | python script/gen_requirements_all.py validate ******* ERROR requirements_all.txt is not up to date Please run script/gen_requirements_all.py ERROR: InvocationError: '/home/travis/build/home-assistant/home-assistant/.tox/requirements/bin/python script/gen_requirements_all.py validate' And what I understand is that a file called requirements_all.txt needs to be updated… is there a document that you can point me to do this? I found a brief reference to it on the developer documentation, but I really don’t understand what I have to do. Thanks in advance! Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10 From: Pascal Vizeli<mailto:notifications@github.com> Sent: Friday, October 13, 2017 4:56 AM To: home-assistant/home-assistant<mailto:home-assistant@noreply.github.com> Cc: Gerardo Castillo<mailto:altersis@hotmail.com>; Mention<mailto:mention@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Update the Russound RNET component to use enhanced Russound.py (#9739) Ping -> Fix travis please — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#9739 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AeprTcl6bjnZzCE6QoZyBKNy1f1QsDUlks5sryWtgaJpZM4PxhXl>.

altersis · 2017-10-19T16:39:01Z

with the help of @armills, I finally got to understand what was missing here. Thanks! what comes next?

emlove

LGTM, but added one small note on code style.

emlove · 2017-10-19T17:31:28Z

+        # amount of traffic and speeding up the update process.
+        ret = self._russ.get_zone_info('1', self._zone_id, 4)
+        _LOGGER.debug("ret= %s", ret)
+        if ret is not None:


Preference:
Our preferred style is the pattern shown below. This saves us from having to indent the rest of the function. This will also line up the indentation of the comments below with the code.

if ret is None: _LOGGER.error("Could not update status for zone %s", self._zone_id) return _LOGGER.debug("Updating status for zone %s", self._zone_id) …

Hi @armills , thank you for your feedback. Could it be possible to do the merge like this just for this moment, so that I can concentrate on additional enhancements to the Russound RNET component? Right now this component cannot reconnect if there is a communications error (forcing you to a full Hass restart), and would like to make fixing this my first priority. I will have the update ready in a week from now (including this style change). Do you agree?

Yep, that's no problem. Not a blocker to merge. Thanks for the contribution! 🎉

acambitsis · 2017-10-19T17:38:21Z

@armills , @andrey-git as an FYI - I have tested this code on my Russound and it works well. @altersis has walked me through the improvements and from my side I am happy.

laf · 2017-10-21T19:31:25Z

Thanks @altersis, @armills and @acambitsis for this. I've been running this code from the PR for a few days so glad it's now merged in :)

altersis added 3 commits October 7, 2017 18:13

Updated RussoundRNETDevice.update() to call and enhanced function tha…

1588d61

…t reduces network traffic Refer to issue #6 on the Russound project

Updated RussoundRNETDevice.update() to invoke an enhanced function to…

6f55614

… reduce network traffic PLease see issue #6 on the russound project

Updated REQUIREMENTS to use version 0.1.9 of the Russound component

83b1bc3

Please refer to issue #6 on the Russound rnet project

homeassistant added merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. platform: media_player.russound_rnet cla-needed labels Oct 7, 2017

houndci-bot reviewed Oct 7, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Oct 7, 2017

Corrected some minor style details to satisfy Houndbot

9e4b003

homeassistant added the cla-signed label Oct 7, 2017

altersis commented Oct 7, 2017

View reviewed changes

fabaff changed the base branch from master to dev October 8, 2017 18:17

fabaff added Hacktoberfest and removed merging-to-master This PR is merging into the master branch and should probably change the branch to `dev`. labels Oct 8, 2017

Update requirements_all.txt

64519f7

altersis requested a review from andrey-git as a code owner October 19, 2017 16:05

homeassistant added the cla-signed label Oct 19, 2017

emlove approved these changes Oct 19, 2017

View reviewed changes

emlove merged commit 5df985a into home-assistant:dev Oct 21, 2017

balloob mentioned this pull request Nov 2, 2017

0.57 #10293

Merged

home-assistant locked and limited conversation to collaborators Mar 3, 2018

ghost removed the platform: media_player.russound_rnet label Mar 21, 2019

ghost added the integration: russound_rnet label Mar 21, 2019

		self._source = None

		_LOGGER.error("Could not update status for zone %s", self._zone_id)

Uh oh!

Conversation

altersis commented Oct 7, 2017

Description:

Example entry for configuration.yaml (if applicable): No configuration.yaml changes are necessary

Checklist:

Uh oh!

homeassistant commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

altersis left a comment

Choose a reason for hiding this comment

Uh oh!

pvizeli commented Oct 13, 2017

Uh oh!

altersis commented Oct 16, 2017

Uh oh!

altersis commented Oct 19, 2017 via email

Uh oh!

altersis commented Oct 19, 2017

Uh oh!

emlove left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acambitsis commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laf commented Oct 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Example entry for `configuration.yaml` (if applicable): No configuration.yaml changes are necessary

acambitsis commented Oct 19, 2017 •

edited

Loading