Split washer and dryer class #96
Conversation
|
I think she's ready for review... |
|
Hey @lllucius ! Is anything else pending from your side or is it ready for review again?
(or just leave a comment, both work :D ) |
|
I kept looking for the "rerequest"...never would have expected it to be a "refresh" button. :-) Thanks for pointing it out. |
whirlpool/dryer.py
Outdated
| ATTR_STATUS_EXTRA_POWER_CHANGEABLE = "Cavity_ChangeStatusExtraPowerChangeable" | ||
| ATTR_STATUS_STEAM_CHANGEABLE = "Cavity_ChangeStatusSteamChangeable" | ||
| ATTR_STATUS_CYCLE_SELECT = "DryCavity_ChangeStatusCycleSelect" | ||
| ATTR_STATUS_DRYNESS = "DryCavity_ChangeStatusDryness" |
There was a problem hiding this comment.
DryCavity_ChangeStatusDryness is a status notification the the Dryness setting has changed.
In what sense? In what situations does it go from 0 to 1 and vice-versa? When you change the dryness level? Or is when when the machine reaches the desired dryness (meaning that clothes are dry, I guess)?
(I am just trying to understand their usage in order to improve the names of the methods and consts, to make it clearer for anyone reading the code)
tests/test_dryer.py
Outdated
| assert dryer.get_status_extra_steam_changeable() == 1 | ||
| assert dryer.get_status_cycle_select() == 0 | ||
| assert dryer.get_status_dryness() == 1 | ||
| assert dryer.get_status_manual_dry_time() == 1 | ||
| assert dryer.get_status_temperature() == 1 | ||
| assert dryer.get_status_wrinkle_shield() == 1 |
There was a problem hiding this comment.
lets remove the status_ from these, since it does not seem to add much context. maybe add _enabled as a suffix? at least for the ones that are setting toggles
There was a problem hiding this comment.
Not really sure about these. It "seems" that they are telling the caller whether that specific attribute can be changed or not. Maybe they should be more like:
assert dryer.get_extra_steam_changeable() == 1
assert dryer.get_cycle_select_changeable() == 0
assert dryer.get_dryness_changeable() == 1
assert dryer.get_manual_dry_time_changeable() == 1
assert dryer.get_temperature_changeable() == 1
assert dryer.get_wrinkle_shield_changeable() == 1
There was a problem hiding this comment.
ah! they inform the app if it should allow the user to change those settings trough the app?
if so, lets rename it like you suggest then.
There was a problem hiding this comment.
these where not updated.
btw, if I got it right, these tell if the user can change the respective settings on the app?
so, if get_status_wrinkle_shield() == False, then ATTR_WRINKLE_SHIELD is read only?
whirlpool/dryer.py
Outdated
| ATTR_CYCLE_SELECT = "DryCavity_CycleSetCycleSelect" | ||
|
|
||
| ATTRVAL_CYCLE_SELECT_REGULAR = "1" |
There was a problem hiding this comment.
| ATTR_CYCLE_SELECT = "DryCavity_CycleSetCycleSelect" | |
| ATTRVAL_CYCLE_SELECT_REGULAR = "1" | |
| ATTR_SELECTED_CYCLE_ = "DryCavity_CycleSetCycleSelect" | |
| ATTRVAL_SELECTED_CYCLE_REGULAR = "1" |
same for the method to make them a bit more explicit on what they mean
There was a problem hiding this comment.
But, changing it to "SELECTED_CYCLE" would lead me to believe it is going to give me what the currently selected cycle is. Whereas, "CYCLE_SELECT" is saying that I'm going to be selecting a specific cycle.
There was a problem hiding this comment.
I am not sure I understood what you mean with
"CYCLE_SELECT" is saying that I'm going to be selecting a specific cycle.
Since this is a getter, isn't it telling what the currently selected cycle is?
There was a problem hiding this comment.
If we look at the data model for this attribute, it show that it's RW, so you can get it and set it. Not sure how you'd like to handle that...
There was a problem hiding this comment.
Right. But even if it also has a setter, it is used to set the selected cycle. As in, "set the selected cycle", not "set the cycle select". It allows you to read/write the selected cycle.
So it would be get_selected_cycle() and set_selected_cycle().
tests/test_dryer.py
Outdated
| assert dryer.get_status_extra_steam_changeable() == 1 | ||
| assert dryer.get_status_cycle_select() == 0 | ||
| assert dryer.get_status_dryness() == 1 | ||
| assert dryer.get_status_manual_dry_time() == 1 | ||
| assert dryer.get_status_temperature() == 1 | ||
| assert dryer.get_status_wrinkle_shield() == 1 |
There was a problem hiding this comment.
ah! they inform the app if it should allow the user to change those settings trough the app?
if so, lets rename it like you suggest then.
whirlpool/dryer.py
Outdated
| ATTR_CYCLE_SELECT = "DryCavity_CycleSetCycleSelect" | ||
|
|
||
| ATTRVAL_CYCLE_SELECT_REGULAR = "1" |
There was a problem hiding this comment.
I am not sure I understood what you mean with
"CYCLE_SELECT" is saying that I'm going to be selecting a specific cycle.
Since this is a getter, isn't it telling what the currently selected cycle is?
whirlpool/dryer.py
Outdated
| ATTR_STATUS_EXTRA_POWER_CHANGEABLE = "Cavity_ChangeStatusExtraPowerChangeable" | ||
| ATTR_STATUS_STEAM_CHANGEABLE = "Cavity_ChangeStatusSteamChangeable" | ||
| ATTR_STATUS_CYCLE_SELECT = "DryCavity_ChangeStatusCycleSelect" | ||
| ATTR_STATUS_DRYNESS = "DryCavity_ChangeStatusDryness" |
There was a problem hiding this comment.
maybe @NodeJSmith can help? I think we should make it more clear what these two attributes and the getters mean.
whirlpool/dryer.py
Outdated
| ATTR_CYCLE_SELECT = "DryCavity_CycleSetCycleSelect" | ||
|
|
||
| ATTRVAL_CYCLE_SELECT_REGULAR = "1" |
There was a problem hiding this comment.
Right. But even if it also has a setter, it is used to set the selected cycle. As in, "set the selected cycle", not "set the cycle select". It allows you to read/write the selected cycle.
So it would be get_selected_cycle() and set_selected_cycle().
tests/test_dryer.py
Outdated
| assert dryer.get_status_extra_steam_changeable() == 1 | ||
| assert dryer.get_status_cycle_select() == 0 | ||
| assert dryer.get_status_dryness() == 1 | ||
| assert dryer.get_status_manual_dry_time() == 1 | ||
| assert dryer.get_status_temperature() == 1 | ||
| assert dryer.get_status_wrinkle_shield() == 1 |
There was a problem hiding this comment.
these where not updated.
btw, if I got it right, these tell if the user can change the respective settings on the app?
so, if get_status_wrinkle_shield() == False, then ATTR_WRINKLE_SHIELD is read only?
|
By the way @lllucius , please feel free to reach out to me on Discord! I go by the same username there (and you can also find me on the HA discord server). |
Sorry @abmantis. I'm a bit lost with what were the last changes you wanted. If you can point thme out, I'll get them changed. Once we finish up with this PR, I'll be bowing out since I'd only built the HA machine to allow my wife to control our dryer. But as I've created the Alexa skill and it's doing what she needs, I'll be reusing the HA box for other purposes. |
Its only these 2: #96 (review) Btw, you can click on the comment date to jump to the original conversation and see what the comment was about:
Oh, I am sad to hear that since you were doing some great work on the library! Hope to see you back someday. |
|
@mkmer @NodeJSmith maybe one of you could give your opinion regarding these two pending comments? I don't have a washer or dryer, so its hard for me to understand how these are used. It would be great if you can help so we could merge this and move into getting more entities into the integration. |
While I don't own a dryer, I do have a washing machine - It will be a couple of weeks before I can test it (if that's what you are looking for). Maybe "So it would be get_selected_cycle() and set_selected_cycle()." would make more sense as |
…xth-sense into split_washer_dryer
|
So we can move this forward, I've renamed the CycleStatus attributes/methods to have the Changeable suffix, since that is what they appear to represent. I've also changed CycleSelect to just Cycle. Please let me know if something is not correct. I don't expect to expose the "Changeable" ones in HA soon, so we have time to fix it anyway. |
This is a resubmit of splitting the washer and dryer class into individual classes.