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

Support to play line in from another Sonos player with test #460

Merged
merged 3 commits into from Mar 18, 2017

Conversation

Projects
None yet
4 participants
@ghcs27
Copy link
Member

commented Jan 1, 2017

This is a copy of #300 with the requested test case for the parameter.
Can someone please check my test code (it's my first test with Mock)?

@coveralls

This comment has been minimized.

Copy link

commented Jan 1, 2017

Coverage Status

Coverage decreased (-0.2%) to 59.948% when pulling d8a7547 on ghcs27:switch_to_line_in into 2f4b577 on SoCo:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

commented Jan 1, 2017

Coverage Status

Coverage decreased (-0.2%) to 59.948% when pulling d8a7547 on ghcs27:switch_to_line_in into 2f4b577 on SoCo:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 1, 2017

Coverage Status

Coverage decreased (-0.2%) to 59.948% when pulling d8a7547 on ghcs27:switch_to_line_in into 2f4b577 on SoCo:master.

@coveralls

This comment has been minimized.

Copy link

commented Feb 23, 2017

Coverage Status

Coverage increased (+1.0%) to 61.08% when pulling d8a7547 on ghcs27:switch_to_line_in into 2f4b577 on SoCo:master.

@KennethNielsen
Copy link
Member

left a comment

Looks good. A few minor comments about updates to the docstring that doesn't really pertain to this PR, but that we might as well get fixed while we are at it. I think the tests look good.

Args:
source (SoCo): The speaker whose line-in should be played.
Default is line-in from the speaker itself.
Returns:

This comment has been minimized.

Copy link
@KennethNielsen

KennethNielsen Mar 16, 2017

Member

This part of the docstring appears to obsolete. We switched a ling time ago away from returning booleans in indicate success. I know you didn't introduce it, but might as well update while we are at it. You can just delete the returns part.

@@ -931,10 +935,14 @@ def switch_to_line_in(self):
Raises SoCoException (or a subclass) upon errors.

This comment has been minimized.

Copy link
@KennethNielsen

KennethNielsen Mar 16, 2017

Member

The Raises stuff is also Napoleon syntax but should be written in the same way as the Returns section i.e:

"""
...
Raises:
    SoCoException: (or a subclass) upon errors.
"""

and this raises section I think should be placed after the args sections
@@ -931,10 +935,14 @@ def switch_to_line_in(self):
Raises SoCoException (or a subclass) upon errors.
"""
if not source:

This comment has been minimized.

Copy link
@KennethNielsen

KennethNielsen Mar 16, 2017

Member

Since we are relying on the boolean value of a SoCo instance anyway, why not turn it around:

if source:
    ...
else:
    ....
@@ -499,6 +499,16 @@ def test_switch_to_line_in(self, moco_zgs):
('CurrentURIMetaData', '')]
)

moco_zgs.avTransport.reset_mock()

This comment has been minimized.

Copy link
@KennethNielsen

KennethNielsen Mar 16, 2017

Member

This tests here look good.

@coveralls

This comment has been minimized.

Copy link

commented Mar 17, 2017

Coverage Status

Coverage increased (+0.6%) to 60.722% when pulling b4c7ef5 on ghcs27:switch_to_line_in into 2f4b577 on SoCo:master.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Looks good, merged. Thanks for the contribution. Can you add a comment about this change to the release notes issue #440

@KennethNielsen KennethNielsen merged commit 90aaf2e into SoCo:master Mar 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghcs27 ghcs27 referenced this pull request Mar 19, 2017

Closed

Release notes for 0.13 #440

@ghcs27 ghcs27 deleted the ghcs27:switch_to_line_in branch Mar 19, 2017

@stefankoegl stefankoegl added this to the 0.13 milestone Nov 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.