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

Code cleanup and improved typings #3072

Closed

Conversation

ericanderson
Copy link

This is a code cleanup.

src/api.ts Show resolved Hide resolved
src/api.ts Show resolved Hide resolved
src/bridgeService.ts Show resolved Hide resolved
src/bridgeService.ts Show resolved Hide resolved
src/logger.ts Show resolved Hide resolved
src/platformAccessory.ts Show resolved Hide resolved
src/platformAccessory.ts Show resolved Hide resolved
src/platformAccessory.ts Show resolved Hide resolved
@Supereg Supereg changed the base branch from master to beta-1.5.0 May 6, 2022 11:04
@coveralls
Copy link

coveralls commented May 6, 2022

Pull Request Test Coverage Report for Build 4645292148

  • 1 of 8 (12.5%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.08%) to 26.114%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/bridgeService.ts 0 3 0.0%
src/logger.ts 1 5 20.0%
Files with Coverage Reduction New Missed Lines %
src/platformAccessory.ts 1 86.67%
src/bridgeService.ts 2 19.93%
src/logger.ts 2 56.94%
Totals Coverage Status
Change from base Build 3575053275: 0.08%
Covered Lines: 392
Relevant Lines: 1355

💛 - Coveralls

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

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

Thanks improving some of the typings of the project 🚀

I had two minor points. Apart from that, this PR looks good to me 👍

@@ -120,7 +126,7 @@ export interface StaticPlatformPlugin extends PlatformPlugin {
*
* @param {(foundAccessories: AccessoryPlugin[]) => void} callback
*/
accessories(callback: (foundAccessories: AccessoryPlugin[]) => void): void;
accessories(callback: (foundAccessories: Array<AccessoryPlugin & AccessoryConfig>) => void): void;
Copy link
Member

@Supereg Supereg May 6, 2022

Choose a reason for hiding this comment

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

I think this typing doesn't really represent reality (and might even break current implementations?).

The weird thing is, that we really only require the name: string property (and optionally check for the uuid_base?: string property). Using AccessoryConfig though will add the accessory: AccessoryName | AccessoryIdentifier requirement and the optional _bridge?: BridgeConfiguration property which both are never used.

Copy link
Member

Choose a reason for hiding this comment

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

Resolving this would make #2815 obsolete and resolve #2771

// @ts-ignore
const uuidBase: string | undefined = accessoryInstance.uuid_base; // optional base uuid

log.info("Initializing platform accessory '%s'...", accessoryName);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove this log statement.

src/logger.ts Show resolved Hide resolved
src/platformAccessory.ts Outdated Show resolved Hide resolved
@Supereg Supereg changed the title Remove ts-ignore usage Code cleanup and improved typings May 6, 2022
@@ -120,7 +126,7 @@ export interface StaticPlatformPlugin extends PlatformPlugin {
*
* @param {(foundAccessories: AccessoryPlugin[]) => void} callback
*/
accessories(callback: (foundAccessories: AccessoryPlugin[]) => void): void;
Copy link

Choose a reason for hiding this comment

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

ericanderson:cleanup-ts-ignore

@Supereg Supereg changed the base branch from beta-1.5.0 to beta-1.5.1 September 21, 2022 04:08
@donavanbecker donavanbecker changed the base branch from beta-1.5.1 to beta-2.0.0 April 8, 2023 12:59
@bwp91
Copy link
Contributor

bwp91 commented Sep 6, 2023

Hi all, just popping into get a status update on this!

@ericanderson and @Supereg any status update on this:?

cc @donavanbecker for context

Cheers, Ben!

@bwp91
Copy link
Contributor

bwp91 commented Sep 21, 2023

@Supereg sorry for tag 😃 how can this PR be edited or resolved for merge?

I like the fact that you commented earlier:

Resolving this would make #2815 obsolete and resolve #2771

@donavanbecker donavanbecker deleted the branch homebridge:beta-2.0.0 October 28, 2023 06:41
@donavanbecker
Copy link
Contributor

@ericanderson, please reopen direct towards beta-1.7.0 branch.

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

5 participants