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 a fake WindowCovering accessory script #165

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

lboue
Copy link

@lboue lboue commented Oct 24, 2018

Should I remove the 2 following function too?

  • def set_position_state(self, value)
  • def set_current_position(self, value)

@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #165 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #165   +/-   ##
=======================================
  Coverage   58.14%   58.14%           
=======================================
  Files          16       16           
  Lines        1639     1639           
  Branches      165      165           
=======================================
  Hits          953      953           
  Misses        652      652           
  Partials       34       34

Copy link
Contributor

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I'm not sure why you've created a completely new file for the Fake WindowCovering. Why not integrate it into busy_home.py?

'TargetPosition', setter_callback=self.set_target_position)

self.char_state = serv_cover.configure_char(
'PositionState', setter_callback=self.set_position_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to assign this char a local variable. The setter_callback is never called, so you can remove it. Maybe add value=2 here as a parameter.

'PositionState', setter_callback=self.set_position_state)

self.char_cur_pos = serv_cover.configure_char(
'CurrentPosition', setter_callback=self.set_current_position)
Copy link
Contributor

Choose a reason for hiding this comment

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

The setter_callback is also never called, so it can be removed.

logging.info("WindowCovering TargetPosition value: %s", value)
self.get_service('WindowCovering')\
.get_characteristic('TargetPosition')\
.set_value(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't set the char to the value that it posted. This isn't necessary. Just remove it.

logging.info("WindowCovering CurrentPosition value: %s", value)
self.get_service('WindowCovering')\
.get_characteristic('CurrentPosition')\
.set_value(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

You've assigned CurrentPosition a local var. Why not use it:

self.char_cur_pos.set_value(value)

logging.info("WindowCovering CurrentPosition value: %s", value)
self.get_service('WindowCovering')\
.get_characteristic('CurrentPosition')\
.set_value(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

The methods set_position_state and set_current_position can be removed, since they are never called.

self.set_info_service(firmware_revision=2, manufacturer="Brand",
model="Shutter", serial_number="0123456789")

# Add the WindowCovering service. Also add optional characteristics to it.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment and the one about assigning accessory infos can be removed as well.

@ikalchev
Copy link
Owner

ikalchev commented Nov 1, 2018

@lboue Will you be addressing the raised issues or should I take care of these after a merge maybe?

@cdce8p
Copy link
Contributor

cdce8p commented Nov 2, 2018

@ikalchev I don't like the idea of merging a PR that isn't finished yet, only to finish it later. Instead consider committing the changes directly to the PR branch. Maintainers do generally have the permission to do so.

@lboue
Copy link
Author

lboue commented Nov 5, 2018

OK. I will fix all that items ASAP.

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.

4 participants