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

fix: Fix Bosch BTH-RA Home Assistant discovery #7720

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

Koenkk
Copy link
Owner

@Koenkk Koenkk commented Jul 1, 2024

@gpayer
Copy link

gpayer commented Jul 1, 2024

Looks good! 👍

@@ -209,7 +209,7 @@ export interface DefinitionMeta {
/**
* Override the Home Assistant discovery payload using a custom function.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a line that informs on where this is executed?

    /**
     * Override the Home Assistant discovery payload using a custom function.
     * Executed right before the payload is passed to `stringify`, before publishing.
     */

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Maybe mentioning the homeassistant.ts extension explicitly would be helpful?

Copy link
Contributor

@burmistrzak burmistrzak left a comment

Choose a reason for hiding this comment

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

Stellar improvement, LGTM! 🤝

@@ -209,7 +209,7 @@ export interface DefinitionMeta {
/**
* Override the Home Assistant discovery payload using a custom function.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Maybe mentioning the homeassistant.ts extension explicitly would be helpful?

@@ -1354,20 +1354,20 @@ const definitions: Definition[] = [
vendor: 'Bosch',
description: 'Radiator thermostat II',
meta: {
overrideHaConfig: (configs) => {
const entry = configs.findIndex((e) => e.type === 'climate');
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better to just pass a single payload, and not all at once. Like it!

@Koenkk Koenkk merged commit 14062ae into master Jul 2, 2024
3 checks passed
@Koenkk Koenkk deleted the fix/bthra-ha-discovery-payload branch July 2, 2024 15:57
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.

4 participants