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

Update library to Python 3.5+ and asyncio #11

Merged
merged 7 commits into from
Nov 10, 2018

Conversation

bachya
Copy link
Contributor

@bachya bachya commented Oct 23, 2018

Hi there! Following up on our conversation here, this PR updates the library to be asyncio-friendly. It also updates the architecture to be more modular and friendly for future additions.

In addition to the library changes, this PR adds a few nice tooling changes that make future contributions easier:

  • A Pipfile
  • A Makefile
  • Various tools to check linting, typing, etc.
  • A more standard setup.py

Lastly, this bumps the version to 1.0.0. I chose that number since this is a very breaking update and usually, the first digit gets bumped in that case. Totally open to your wishes, though.

My suggestion as to how to proceed:

  1. Let's work through this PR and get it merged.
  2. Let's open a new Home Assistant PR that focuses simply on transitioning the existing functionality to this new version (I have one ready to go).
  3. You rebase MyQ updates, scan_interval, status retries home-assistant/core#17535 on top of step 2, fix any issues that arise, and proceed with getting that PR merged.

Thoughts?

@ehendrix23
Copy link
Collaborator

@arraylabs, @bachya .

Would it make sense to use requests.session instead of requests.get and requests.post? That would allow the TCP connection to stay open and thus no need to keep on recreating it.
I just submitted a PR for august (
snjoetw/py-august#11) and it was fairly easy to do. Looking at MyQ I would say it should be fairly simple here as well.

The difference for August from my home was 294ms with request.get compared to 80ms for request.session. Thoughts?

@bachya
Copy link
Contributor Author

bachya commented Oct 23, 2018

@ehendrix23 That's definitely the correct principle. Note that this PR does away with requests altogether: since it attempts to make the library asyncio-compatible, it replaces requests with aiohttp. You can see here that it uses a ClientSession, which accomplishes what you're talking about.

@ehendrix23
Copy link
Collaborator

@bachya Well, that teaches me to just have a look at existing code and the description of this PR. :-)

ehendrix23 and others added 2 commits October 23, 2018 18:20
Added retries for open and close as I've seen this fail as well.
Changed so that update, open, and close return True if successful or False if unsuccessful.
Added retry for open/close and True/False return for update, close, open.
@vajonam
Copy link

vajonam commented Nov 6, 2018

can I help with this? would like to get this back into home assistant as working component.

@bachya
Copy link
Contributor Author

bachya commented Nov 6, 2018

@vajonam: Waiting on @arraylabs to approve.

Copy link
Owner

@arraylabs arraylabs left a comment

Choose a reason for hiding this comment

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

Wow, that appears to have been an undertaking! Will take me a bit to wrap my head around all of it. Thanks for the hard work and I apologize for the delay in getting back to it as I've been traveling again.

@arraylabs arraylabs merged commit 0ace49e into arraylabs:master Nov 10, 2018
@bachya
Copy link
Contributor Author

bachya commented Nov 10, 2018

No problem, @arraylabs! Are you good if I start on number 2 in my checklist above?

@arraylabs
Copy link
Owner

@bachya yes please do!

@bachya
Copy link
Contributor Author

bachya commented Nov 15, 2018

I’ll need you to publish a new version on PyPi. Let me know when that’s in place. 👍🏻

@arraylabs
Copy link
Owner

@bachya it should be there now, fyi adding the publish/upload to the setup.py just makes things too easy. I should have thought about that about 15 versions ago...

@ehendrix23
Copy link
Collaborator

@arraylabs if you would have done it 15 versions ago then you would not be appreciating how easy that makes it now. :-)

@bachya, let me know when you have something for testing. As long as my garage doors won't start opening and closing on random moments I'm good. :-)

@bachya
Copy link
Contributor Author

bachya commented Nov 15, 2018

@ehendrix23 FYI, I've submitted a PR to HASS: home-assistant/core#18489 – you can use that for testing.

@ehendrix23
Copy link
Collaborator

@bachya,

OK, just put it in my HASS. Working so far. :-)

Noticed though that there are no debug logger messages. I might see to add some in both pymyq and the HASS component once we've got this PR in.
Those can help when something isn't working as it should. :-)

@vajonam
Copy link

vajonam commented Nov 16, 2018

I am using this as well, works fine this far. no issues to report

@bachya bachya deleted the dev-update branch August 6, 2019 21:25
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