homebridge / HAP-NodeJS

Node.js implementation of the HomeKit Accessory Protocol (HAP)
Apache License 2.0
2.68k stars 630 forks source link

AdaptiveLightingController fix & improvement #1038

Closed Shaquu closed 3 months ago

Shaquu commented 3 months ago

:recycle: Current situation

Describe the current situation. Explain current problems, if there are any. Be as descriptive as possible (e.g., including examples or code snippets).

Would be great to have update data provided from update event. Also noticed few type issues, now properly using optional chaining.

:bulb: Proposed solution

Describe the proposed solution and changes. How does it affect the project? How does it affect the internal structure (e.g., refactorings)?

Would be great to have update data provided from update event. Also noticed few type issues, now properly using optional chaining.

:gear: Release Notes

Provide a summary of the changes or features from a user's point of view. If there are breaking changes, provide migration guides using code examples of the affected features.

AdaptiveLightingController: Update payload added to update event. AdaptiveLightingController: Resolve possible TypeError, related to incorrect optional values unwrapping.

:heavy_plus_sign: Additional Information

If applicable, provide additional context in this section.

Testing

Which tests were added? Which existing tests were adapted/changed? Which situations are covered, and what edge cases are missing?

N/A

Reviewer Nudging

Where should the reviewer start? what is a good entry point?

Adaptive Lightning API :)

Shaquu commented 3 months ago

No clue why some checks failed. typedoc complains about @since iOS 14 which is not scope of my change pr labeler cannot start at all

Shaquu commented 3 months ago

Ahh @donavanbecker I see I did the same as you. Not sure if good. Should it target latest or beta?

donavanbecker commented 3 months ago

Ahh @donavanbecker I see I did the same as you. Not sure if good. Should it target latest or beta?

lets do beta. We can then do a quick Homebridge beta before release this version.

bwp91 commented 3 months ago
Shaquu commented 3 months ago

Good point. Let me check for both, let's not rush it.

bwp91 commented 3 months ago

also if you update your branch from the beta branch, you should find the build issues are all fixed πŸ‘

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9649422541

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/controller/AdaptiveLightingController.ts 0 17 0.0%
<!-- Total: 0 17 0.0% -->
Files with Coverage Reduction New Missed Lines %
src/lib/Accessory.ts 3 89.29%
<!-- Total: 3 -->
Totals Coverage Status
Change from base Build 9649015627: -0.3%
Covered Lines: 7411
Relevant Lines: 10616

πŸ’› - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9652898208

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/controller/AdaptiveLightingController.ts 0 17 0.0%
<!-- Total: 0 17 0.0% -->
Totals Coverage Status
Change from base Build 9649015627: -0.2%
Covered Lines: 7414
Relevant Lines: 10616

πŸ’› - Coveralls
Shaquu commented 3 months ago

@bwp91

  • this won't cause any issues with existing plugins that use adaptive lighting API right? (just wanted to confirm)

With the new parameter in listener, you can still use listener without a parameter. So current implementations should be all right.

  • it would be nice if you could update src/accessories/Light-AdaptiveLighting_accessory.ts with perhaps just a simple way of showing how the new update information can be retrieved

Updated, but as AUTOMATIC is default I put some comments and some code.

Shaquu commented 3 months ago

Anyone, PR Labeler / stale / update_release_draft (pull_request) failing image

coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9652969910

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/controller/AdaptiveLightingController.ts 0 17 0.0%
<!-- Total: 0 17 0.0% -->
Totals Coverage Status
Change from base Build 9652967305: -0.2%
Covered Lines: 7414
Relevant Lines: 10616

πŸ’› - Coveralls
coveralls commented 3 months ago

Pull Request Test Coverage Report for Build 9653108648

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/lib/controller/AdaptiveLightingController.ts 0 17 0.0%
<!-- Total: 0 17 0.0% -->
Totals Coverage Status
Change from base Build 9652967305: -0.2%
Covered Lines: 7414
Relevant Lines: 10616

πŸ’› - Coveralls