Skip to content

Adds customizing of device port in TPLinkSmartHomeProtocol.query #109

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.

Sign up for GitHub

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

Jump to bottom
Closed
kirichkov wants to merge 2 commits into from
Closed

Adds customizing of device port in TPLinkSmartHomeProtocol.query #109

kirichkov wants to merge 2 commits into from

Conversation

@kirichkov
Copy link
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

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
kasa/protocol.py Outdated
async def query(
host: str,
request: Union[str, Dict],
retry_count: int = 3,
Copy link
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
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
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
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
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
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