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

Add set_states method #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

grahamhome
Copy link

This PR simply adds a method for the Lifx "Set States" endpoint so that multiple lights can be set simultaneously. It also addresses a defect in "client.py" which was preventing json_body parameters from being sent in requests to the Lifx API.

@grahamhome
Copy link
Author

@cydrobolt I am really enjoying using your Pifx library. I needed to add a method for an endpoint that wasn't in the library. Will you consider accepting my PR? Please let me know if you'd like me to make any other changes to this PR. Thanks in advance!

pifx/core.py Outdated
@@ -72,6 +72,41 @@ def set_state(self, selector='all',
method='put', endpoint='lights/{}/state',
endpoint_args=[selector], argument_tuples=argument_tuples)

def set_states(self, states=[{'selector': 'all',
Copy link
Owner

@cydrobolt cydrobolt Aug 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default argument seems potentially error-prone. See https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

It might be better to write states=None and then if states is None: states = [] in the method itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cydrobolt I have updated the method to remove the mutable default argument. I added a usage example in the docstring so users will hopefully know how to call it. Let me know if you'd like to see any other changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants