-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add time platform for eheimdigital #38603
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
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for home-assistant-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
📝 WalkthroughWalkthroughThe documentation for the EHEIM Digital integration was updated to include support for time-based configuration entities. The integration metadata now lists "Time" as both a category and a platform. New sections were added to the documentation for the EHEIM thermocontrol+e and EHEIM classicVARIO+e devices, describing "Day start time" and "Night start time" entities that allow configuration of day and night settings for temperature and pump speed in Bio mode. No existing entities or functionality were removed or changed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeAssistant
participant EHEIM_Device
User -> HomeAssistant: Set "Day start time" or "Night start time"
HomeAssistant -> EHEIM_Device: Update Bio mode configuration with new time
EHEIM_Device -> HomeAssistant: Confirm configuration update
HomeAssistant -> User: Reflect updated time entity state
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
source/_integrations/eheimdigital.markdown (2)
61-65
: Document the expected time format for Bio mode temperature
Great addition of the Time section under the EHEIM thermocontrol+e device. To improve clarity for end users, consider specifying the required time input format (e.g.,HH:MM
) or linking to the Home Assistant time entity documentation.
80-84
: Document the expected time format for Bio mode pump speed
The new Time section for the EHEIM classicVARIO+e device is helpful. As a refinement, you could mention the unit or format (for example,HH:MM
), and note any default values or acceptable range constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/_integrations/eheimdigital.markdown
(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - home-assistant-docs
- GitHub Check: Header rules - home-assistant-docs
- GitHub Check: Pages changed - home-assistant-docs
🔇 Additional comments (1)
source/_integrations/eheimdigital.markdown (1)
4-9
:❓ Verification inconclusive
Time category and platform entries look correct
The new “Time” entry inha_category
and the corresponding lowercasetime
inha_platforms
are inserted in alphabetical order and follow existing conventions. Please verify that theeheimdigital
integration’s manifest.json also declares"time"
under itsplatforms
so the documentation and implementation remain in sync.Also applies to: 17-22
🏁 Script executed:
#!/bin/bash # Verify that 'time' is declared under platforms in manifest.json grep -R '"platforms":' manifest.json -A 5 | grep '"time"'Length of output: 106
🏁 Script executed:
#!/bin/bash # Locate all manifest.json files in the repo echo "Searching for manifest.json files..." fd manifest.json # Filter for the eheimdigital integration’s manifest manifest=$(fd manifest.json | grep -i eheimdigital | head -n1) if [ -z "$manifest" ]; then echo "Error: Could not find eheimdigital/manifest.json" exit 1 fi echo "Using manifest file: $manifest" # Show the platforms section for context echo -e "\nPlatforms section in manifest:" grep -R '"platforms"' "$manifest" -A 5 # Verify that 'time' is declared under platforms echo -e "\nChecking for '\"time\"' in platforms:" grep -R '"time"' "$manifest" || echo "'time' not found in platforms"Length of output: 301
Ensure
time
platform is declared in the integration manifest
I don’t see amanifest.json
for the eheimdigital integration in this documentation repo. Please manually verify in the integration’s code (for example undercustom_components/eheimdigital/manifest.json
orhomeassistant/components/eheimdigital/manifest.json
) that the"platforms"
array includes"time"
, so the docs and implementation stay in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Approved. Can be merged as soon as the parent PR gets merged.
Proposed change
This adds the time platform to eheimdigital.
Type of change
current
branch).current
branch).next
branch).next
branch).Additional information
Checklist
current
branch.next
branch.Summary by CodeRabbit