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

Cleanup - type hints, remove unused imports, remove disabled auto-renewal logic, add pre-commit, etc #52

Closed
wants to merge 12 commits into from

Conversation

NodeJSmith
Copy link
Contributor

@NodeJSmith NodeJSmith commented Apr 8, 2024

Wanted to get things cleaned up a bit before working on the EventSocket logic, to make sure that PR didn't include extraneous stuff.

Cleanup includes:

  • add .flake8 file to ignore E402 (imports not at top of file, due to HA license comment in tests/aiohttp)
  • remove comment from pyproject exclude line
  • switch out Dict/List imports from typing with dict and list
  • add type hints to all (?) code in auth, appliance, and appliancesmanager
  • reorganized the methods in Appliance - this one was purely for my own benefit, I wanted to get all the ones that use EventSocket into one contiguous section
  • remove the __init__ methods from WasherDryer, Aircon, and Oven - I can add these back if necessary, but I noticed that they weren't actually doing anything different from the Appliance __init__ method so it seemed like they didn't need to be there
  • in the cli, dump the raw data using json.dumps so it's easier to read

Let me know if there are any concerns with these changes or if there are formatting conventions that this project uses that I may have missed.

Note: I didn't bump the version since there are no actual functionality changes and figured we wouldn't both bumping until there were new features/changes.

@NodeJSmith
Copy link
Contributor Author

Only set WP-CLIENT-BRAND header on shared appliances call. Bump to 0.18.8 due to that change.

(hopefully) Fixes #home-assistant/core#115130

@abmantis Can I get your eyes on this when you have a moment? And if you'd prefer I do the HA fix in a PR without any cleanup I can split these, just let me know

@NodeJSmith
Copy link
Contributor Author

Confirmed on the HA issue that this change does fix the issue, so we'll definitely want this in as quickly as possible

@abmantis
Copy link
Owner

abmantis commented Apr 8, 2024

Thanks for this!
Yeah, it would be great if you could split this into smaller PRs, each focused on a single change. Otherwise this will take a lot to get merged, since it will get more review rounds.

Also, it is easier to find time during the day to review small PRs :)

@abmantis
Copy link
Owner

abmantis commented Apr 8, 2024

It is fine to leave the version bump out as that allows us to bundle more changes if needed, and I can do it anyway for the release

@NodeJSmith NodeJSmith closed this Apr 9, 2024
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.

2 participants