Skip to content

Adds customizing of device port in TPLinkSmartHomeProtocol.query#109

Closed
kirichkov wants to merge 2 commits intopython-kasa:masterfrom
kirichkov:allow-using-custom-port-in-query
Closed

Adds customizing of device port in TPLinkSmartHomeProtocol.query#109
kirichkov wants to merge 2 commits intopython-kasa:masterfrom
kirichkov:allow-using-custom-port-in-query

Conversation

@kirichkov
Copy link
Copy Markdown
Contributor

@kirichkov kirichkov commented Oct 31, 2020

This addresses the changing of destination port. Requested in #108

@kirichkov kirichkov force-pushed the allow-using-custom-port-in-query branch from f851ba5 to 134efce Compare October 31, 2020 14:15
@kirichkov kirichkov mentioned this pull request Oct 31, 2020
Closed
@kirichkov kirichkov force-pushed the allow-using-custom-port-in-query branch from 134efce to 79faf8e Compare October 31, 2020 14:21
@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 31, 2020 •
edited
Loading

Codecov Report

Merging #109 (7dc4060) into master (70061cb) will increase coverage by 0.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   72.88%   73.15%   +0.26%     
==========================================
  Files          10       10              
  Lines        1232     1233       +1     
  Branches      185      183       -2     
==========================================
+ Hits          898      902       +4     
+ Misses        300      298       -2     
+ Partials       34       33       -1
Impacted Files Coverage Δ
kasa/protocol.py 74.62% <100.00%> (ø)
kasa/smartbulb.py 94.32% <100.00%> (+0.04%) ⬆️
kasa/smartdevice.py 82.78% <100.00%> (+0.05%) ⬆️
kasa/smartdimmer.py 93.22% <100.00%> (+0.11%) ⬆️
kasa/smartlightstrip.py 100.00% <100.00%> (ø)
kasa/smartplug.py 100.00% <100.00%> (ø)
kasa/smartstrip.py 78.35% <100.00%> (+0.16%) ⬆️
kasa/discover.py 40.17% <0.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70061cb...7dc4060. Read the comment docs.

rytilahti
rytilahti reviewed Oct 31, 2020
View reviewed changes
Comment thread kasa/protocol.py Outdated
async def query(
host: str,
request: Union[str, Dict],
retry_count: int = 3,
Copy link
Copy Markdown
Member

@rytilahti rytilahti Oct 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have nothing against this change although I'm not sure how many use cases there are for it, but what do you think about allowing passing the wanted port to __init__? That would allow us to expose this directly to SmartDevice constructors.

Copy link
Copy Markdown
Contributor Author

@kirichkov kirichkov Nov 1, 2020

Choose a reason for hiding this comment

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

I think that's better, actually. I'm waiting for feedback to see the #108 use case.

@kirichkov kirichkov force-pushed the allow-using-custom-port-in-query branch 2 times, most recently from 8f8d6ec to 7dc4060 Compare November 9, 2020 14:28
@kirichkov
Copy link
Copy Markdown
Contributor Author

kirichkov commented Nov 9, 2020 •
edited
Loading

How should we share the DEFAULT_PORT constant? I'm currently importing it everywhere by importing the TPLinkSmartHomeProtocol class. Any suggestions?

@kirichkov kirichkov requested a review from rytilahti November 11, 2020 09:03
@kirichkov kirichkov mentioned this pull request Nov 17, 2020
Closed
@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Nov 20, 2020

Hey and sorry for the belated response, I have been busy with other projects & got hit by flu. Anyway, I'm starting to have second thoughts on this, I would definitely not want to reintroduce the port variable through all the layers as it makes the code more confusing...

My proposal would be to convert TPLinkSmartHomeProtocol.query() into a regular method, so the use case of the original requester could use something like this (after the initialization, before doing any queries):

d = SmartDevice("123.123.123.123")
d.protocol.port = 1234
await d.update()

What do you think about something like this, @kirichkov?

@kirichkov
Copy link
Copy Markdown
Contributor Author

kirichkov commented Nov 24, 2020

No worries! Hope you're doing better!

I second your proposal! I don't like all those port constants all over the place. Hence why I asked for some input :)

I'll get this changed and we can discuss it further.

@kirichkov kirichkov force-pushed the allow-using-custom-port-in-query branch 2 times, most recently from c72ede8 to 71ecbc2 Compare November 29, 2020 16:17
@kirichkov kirichkov force-pushed the allow-using-custom-port-in-query branch from 71ecbc2 to 54bc7fa Compare November 29, 2020 16:23
@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Sep 19, 2021

Considering there has been no action for a while, I think we can close this and reopen if really necessary.

@rytilahti rytilahti closed this Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rytilahti rytilahti Awaiting requested review from rytilahti

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kirichkov @codecov-io @rytilahti