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

Nice G.O. code quality improvements #124319

Merged
merged 17 commits into from
Sep 6, 2024
Merged

Conversation

IceBotYT
Copy link
Contributor

@IceBotYT IceBotYT commented Aug 20, 2024

Proposed change

Code quality improvements before upping to platinum

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Please split the changes from the quality bump. For the quality bump, also include the whole checklist (check other PRs for the quality as example)

homeassistant/components/nice_go/coordinator.py Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft August 20, 2024 17:30
@IceBotYT IceBotYT marked this pull request as ready for review August 20, 2024 17:39
@home-assistant home-assistant bot requested a review from joostlek August 20, 2024 17:39
@IceBotYT IceBotYT changed the title Bring Nice G.O. up to platinum Nice G.O. code quality improvements Aug 20, 2024
@IceBotYT IceBotYT marked this pull request as draft August 27, 2024 01:07
@IceBotYT
Copy link
Contributor Author

Waiting on #124667 to be merged since it is higher priority to resolve errors and disconnection issues before release

@IceBotYT IceBotYT force-pushed the nice-go-platinum branch 2 times, most recently from 5a59ad7 to 9f5ec5e Compare August 27, 2024 18:22
@IceBotYT IceBotYT marked this pull request as ready for review August 27, 2024 18:33
@home-assistant home-assistant bot requested a review from joostlek August 27, 2024 18:33
@mback2k
Copy link
Contributor

mback2k commented Aug 27, 2024

Will this PR also fix the following issue that can currently be observed in the test suite?

name='NiceGOApi()' spec='NiceGOApi' id='140520354727360'>.authenticate_refresh
FAILED tests/components/nice_go/test_diagnostics.py::test_entry_diagnostics - AssertionError: assert [+ received] == [- snapshot]
    dict({
       ...
          'refresh_token': '**REDACTED**',
  -       'refresh_token_creation_time': 1722184160.738171,
  +       'refresh_token_creation_time': 1724787660.163543,
        }),
      ...
    })

@IceBotYT
Copy link
Contributor Author

IceBotYT commented Aug 27, 2024

Yes it will.
This commit fixes it 9f5ec5e

@IceBotYT IceBotYT mentioned this pull request Aug 27, 2024
19 tasks
@IceBotYT IceBotYT marked this pull request as draft August 29, 2024 18:13
@IceBotYT IceBotYT marked this pull request as ready for review August 30, 2024 22:31
jbouwh
jbouwh previously requested changes Aug 31, 2024
Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

Split dependency update from other changes.

Copy link
Contributor

@jbouwh jbouwh left a comment

Choose a reason for hiding this comment

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

LGTM,
Thanks @IceBotYT 👍

@joostlek Can you have a review here too as I added the last commit myself

@jbouwh
Copy link
Contributor

jbouwh commented Sep 6, 2024

@IceBotYT For future PR's, try to keep them as small as possible to make reviewing easier

homeassistant/components/nice_go/coordinator.py Outdated Show resolved Hide resolved
homeassistant/components/nice_go/coordinator.py Outdated Show resolved Hide resolved
tests/components/nice_go/test_init.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft September 6, 2024 12:19
@jbouwh jbouwh marked this pull request as ready for review September 6, 2024 15:57
@home-assistant home-assistant bot requested review from jbouwh and joostlek September 6, 2024 15:57
@jbouwh
Copy link
Contributor

jbouwh commented Sep 6, 2024

Thanks @joostlek and @IceBotYT

@jbouwh jbouwh merged commit cd3059a into home-assistant:dev Sep 6, 2024
28 checks passed
@IceBotYT IceBotYT deleted the nice-go-platinum branch September 6, 2024 19:01
Gigatrappeur pushed a commit to Gigatrappeur/ha-core that referenced this pull request Sep 7, 2024
* Bring Nice G.O. up to platinum

* Switch to listen in coordinator

* Tests

* Remove parallel updates from coordinator

* Unsub from events on config entry unload

* Detect WS disconnection

* Tests

* Fix tests

* Set unsub to None after unsubbing

* Wait 5 seconds before setting update error to prevent excessive errors

* Tweaks

* More tweaks

* Tweaks part 2

* Potential test for hass stopping

* Improve reconnect handling and test on Homeassistant stop event

* Move event handler to entry init

* Patch const instead of asyncio.sleep

---------

Co-authored-by: jbouwh <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants