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

Some cleanup #18

Merged
merged 19 commits into from
Jul 1, 2021
Merged

Some cleanup #18

merged 19 commits into from
Jul 1, 2021

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Jun 25, 2021

Made a couple of changes:

  1. There's no need for the partner flag. We can simply check if there is a right user and then if there is a left user that isn't the same as the right user. I think the end result of this is the same... I guess you lose the ability to not include your partner as a user, but I am not sure if that's a common use case.
  2. Added support for getting the client session from HA instead of generating it on our own
  3. Removed the async_timeout dependency because you can pass the timeout directly to the get/put/post with current versions of aiohttp.
  4. Removed loop as an input. I don't think we need to set the event loop, HA will do that for us, and because of the other changes, it's not needed as an input elsewhere.
  5. Made it so that if the client session is created internally by the library, it will get closed automatically on shutdown. I wrote this so that you can safely call stop() multiple times.
  6. Removed api_key since it is not used anywhere
  7. Added aiohttp as a dependency so that users outside of HA can use the package

I have done some brief testing, and things appear to be working as expected. Assuming you are OK with these changes and can cut a new release, I'd be happy to make the corresponding changes in HA since this is a breaking change that I am introducing. I will also likely pick up home-assistant/core#47493 at some point to try to get that over the finish line

EDIT: Just found this comment on Discord, so it seems we are on the same page for item 1 🙂 https://discord.com/channels/330944238910963714/330990195199442944/817744252058337330

@raman325 raman325 changed the title Remove partner boolean and figure out the users automatically Some cleanup Jun 25, 2021
@mezz64
Copy link
Owner

mezz64 commented Jun 25, 2021

Thanks for the PRs! I'll try to find some time to test this weekend, all looks good on a quick glance though.

@mezz64
Copy link
Owner

mezz64 commented Jun 30, 2021

All looks well in testing thus far. Few questions:

  • Since the password encoding fix has been merged can you update this PR to resolve the conflicts
  • Have you been able to test as a single bed user? I know some others had potentially odd behavior in the past when setting things up to use both sides of the mattress as a single sleeper. I think your changes will handle this ok, it's just not something I'm able to test.
  • Can you update the README example and bump the python and package requirements appropriately.

@raman325
Copy link
Contributor Author

raman325 commented Jun 30, 2021

All looks well in testing thus far. Few questions:

  • Since the password encoding fix has been merged can you update this PR to resolve the conflicts

Resolved

  • Have you been able to test as a single bed user? I know some others had potentially odd behavior in the past when setting things up to use both sides of the mattress as a single sleeper. I think your changes will handle this ok, it's just not something I'm able to test.

Yes at the time when I submitted this I had a single user bed and the fixes worked for me.

  • Can you update the README example and bump the python and package requirements appropriately.

Yep, will do that shortly

@raman325
Copy link
Contributor Author

Sorry, what part of the README needs to be updated? I removed the async_timeout dependency since we aren't using that, and I had already updated the example call, am I missing something?

@mezz64
Copy link
Owner

mezz64 commented Jun 30, 2021

Main thing left in the README is the python version. I believe we need at least 3.7 now to support the asyncio.run call.

@raman325
Copy link
Contributor Author

raman325 commented Jul 1, 2021

Done! I also specified a python_requires so pip doesn't install the package on older installations of python

@mezz64 mezz64 merged commit ee6c25e into mezz64:master Jul 1, 2021
@mezz64
Copy link
Owner

mezz64 commented Jul 1, 2021

@raman325 This is now published as 0.1.7 on pypi (missed updating the tarball location in the setup so had to republish). Feel free to move forward with changes on the HA side.

@raman325 raman325 deleted the remove_partner branch July 1, 2021 16:42
@raman325
Copy link
Contributor Author

raman325 commented Jul 1, 2021

Great thanks!

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.

None yet

2 participants