Add core pull request review guide#3113
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Note the per-entity polling exception to using DataUpdateCoordinator, and that raising ConfigEntryAuthFailed requires async_step_reauth to be implemented in the config flow.
Link to the entity description docs and note that other patterns described there are also accepted.
A duplicated list risks falling out of sync with the source of truth.
Drop the draft-PR bullet, trim the wordy intro and per-bullet extended commentary, and remove the confusing closure quote on the AI-generated PR signals bullet.
- Drop the checkboxes from every bullet. - Drop the 🤖 (automatable) and ✅ (CI-checked) markers; keep 👁️ to highlight items that need human judgment. - Apply sentence case to all headings. - Switch em-dashes to the no-space Merriam-Webster style. - Drop the statistical numbers (PR counts, percentages) sprinkled through the bullets and section intros.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/core/pr_review_guide.md (2)
143-143: 💤 Low valueConsider clarifying "many comments" for readability.
The phrase "generate many comments per reviewed PR" is ambiguous without a comparative reference. Consider "generate the most review comments" or "typically generate many review comments" to improve clarity.
✏️ Optional phrasing improvement
-Bugfixes generate many comments per reviewed PR despite being conceptually simpler than other changes—because reviewers scrutinize whether the fix is actually correct and complete. +Bugfixes typically generate many review comments despite being conceptually simpler than other changes—because reviewers scrutinize whether the fix is actually correct and complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/core/pr_review_guide.md` at line 143, Edit the sentence that reads "Bugfixes generate many comments per reviewed PR despite being conceptually simpler than other changes." and replace it with a clearer phrasing such as "Bugfixes typically generate many review comments per reviewed PR despite being conceptually simpler than other changes" or "Bugfixes generate the most review comments per reviewed PR despite being conceptually simpler than other changes" to remove ambiguity; update the string in the same paragraph where that sentence appears (the sentence starting with "Bugfixes generate many comments per reviewed PR") to one of the suggested alternatives.
303-304: 💤 Low valueClarify connection cleanup vs memory release.
The statement "Memory is automatically released when the Home Assistant process terminates" might be misread to suggest that connection cleanup isn't necessary. Consider emphasizing that connections (network sockets, file handles) require explicit cleanup via
EVENT_HOMEASSISTANT_STOP, even though Python memory is auto-released on process exit.✏️ Suggested clarification
-- 👁️ **Close connections on shutdown.** When Home Assistant stops connections should be closed. Listen for the `EVENT_HOMEASSISTANT_STOP` event to schedule the cleanup. Memory is automatically released when the Home Assistant process terminates. +- 👁️ **Close connections on shutdown.** When Home Assistant stops, connections should be closed. Listen for the `EVENT_HOMEASSISTANT_STOP` event to schedule the cleanup. This is necessary for network sockets and file handles even though Python memory is automatically released when the process terminates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/core/pr_review_guide.md` around lines 303 - 304, Update the "Close connections on shutdown" guidance to explicitly distinguish connection cleanup from memory release: state that while Python memory is reclaimed by the OS when the process ends, network sockets, file handles, open DB connections and similar resources must be explicitly closed on shutdown by listening for EVENT_HOMEASSISTANT_STOP and performing teardown (close(), await client.disconnect(), cancel tasks, etc.), and give a short example suggestion text that callers should schedule their cleanup there. Use the heading "Close connections on shutdown" and the EVENT_HOMEASSISTANT_STOP symbol to locate where to change the wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/core/pr_review_guide.md`:
- Line 143: Edit the sentence that reads "Bugfixes generate many comments per
reviewed PR despite being conceptually simpler than other changes." and replace
it with a clearer phrasing such as "Bugfixes typically generate many review
comments per reviewed PR despite being conceptually simpler than other changes"
or "Bugfixes generate the most review comments per reviewed PR despite being
conceptually simpler than other changes" to remove ambiguity; update the string
in the same paragraph where that sentence appears (the sentence starting with
"Bugfixes generate many comments per reviewed PR") to one of the suggested
alternatives.
- Around line 303-304: Update the "Close connections on shutdown" guidance to
explicitly distinguish connection cleanup from memory release: state that while
Python memory is reclaimed by the OS when the process ends, network sockets,
file handles, open DB connections and similar resources must be explicitly
closed on shutdown by listening for EVENT_HOMEASSISTANT_STOP and performing
teardown (close(), await client.disconnect(), cancel tasks, etc.), and give a
short example suggestion text that callers should schedule their cleanup there.
Use the heading "Close connections on shutdown" and the EVENT_HOMEASSISTANT_STOP
symbol to locate where to change the wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 858dd427-fe18-4c27-a653-2a1c7767804c
📒 Files selected for processing (1)
docs/core/pr_review_guide.md
Markus98
left a comment
Markus98
left a comment
There was a problem hiding this comment.
Besides the comments I left, it looks pretty good to me.
I am not sure if we all aligned yet on whether this is something we want to include in the devdocs, so maybe we should confirm if the majority are onboard with merging this before doing it.
Markus98
left a comment
Markus98
left a comment
There was a problem hiding this comment.
Just a comment on the emoji tag on the review bullet points. After that has been resolved I am good to approve this. I would ideally want this to have at least one more approval in addition to mine before we merge this so that there is some show of a consensus.
|
|
||
| A practical guide for reviewing pull requests on `home-assistant/core`. | ||
|
|
||
| Items marked 👁️ require human judgment and are often the hardest to get right when relying on automated checks. The remaining items may be partially covered by linters, hassfest, CI, or automated reviewers, but are listed here as useful context for reviewers. |
There was a problem hiding this comment.
Maybe we should just get rid of this and the inline emojis altogether? This made sense when we had the other categories, but right now it seems really arbitrary which ones the eye emoji is added to. There are several points which require human judgement that are not marked as such.
| ### 6.3 License compliance | ||
|
|
||
| - **License hasn't changed between versions.** A license change (e.g., MIT → GPL-3.0) between the old and new version would be a critical issue. Compare the `license` field on PyPI for both versions. | ||
| - **License is still Apache-2.0-compatible.** Same rules as new integrations: GPL-2.0/3.0/AGPL are incompatible with HA Core's Apache-2.0 license. |
There was a problem hiding this comment.
Is that already a rule? I don't think we enforce this and it is document nowhere else
There was a problem hiding this comment.
The dependency license must obviously not be incompatible with our own license.
There was a problem hiding this comment.
That does make sense logically. All I am saying we don’t really consider this at the moment and it’s not written explicitly anywhere. I recently asked frenck about this and he said they (you?) were working on solution 🤷♂️
|
|
||
| ### 7.3 Mocking | ||
|
|
||
| - 👁️ **Mock the library, not HA internals.** Mocks should target the third-party library's API calls, not Home Assistant's internal mechanisms. |
There was a problem hiding this comment.
This could use some more details & examples (coordinator, etc.) because "HA internals" is an abstract concept
There was a problem hiding this comment.
Let's follow up with that, in that case.
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
8 participants
Proposed change
Todo
Type of change
Checklist
Additional information
Summary by CodeRabbit