Extend GitHub Copilot instructions with new learnings from reviews (#147652)

This commit is contained in:
Franck Nijhof 2025-06-27 09:01:09 +02:00 committed by GitHub
parent e481f14335
commit 55a37a2936
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

View File

@ -96,8 +96,14 @@ rules:
- **coordinator.py**: Centralize data fetching logic - **coordinator.py**: Centralize data fetching logic
```python ```python
class MyCoordinator(DataUpdateCoordinator[MyData]): class MyCoordinator(DataUpdateCoordinator[MyData]):
def __init__(self, hass: HomeAssistant, client: MyClient) -> None: def __init__(self, hass: HomeAssistant, client: MyClient, config_entry: ConfigEntry) -> None:
super().__init__(hass, logger=LOGGER, name=DOMAIN, update_interval=timedelta(minutes=1)) super().__init__(
hass,
logger=LOGGER,
name=DOMAIN,
update_interval=timedelta(minutes=1),
config_entry=config_entry, # ✅ Pass config_entry - it's accepted and recommended
)
``` ```
- **entity.py**: Base entity definitions to reduce duplication - **entity.py**: Base entity definitions to reduce duplication
```python ```python
@ -203,13 +209,24 @@ rules:
- **Standard Pattern**: Use for efficient data management - **Standard Pattern**: Use for efficient data management
```python ```python
class MyCoordinator(DataUpdateCoordinator): class MyCoordinator(DataUpdateCoordinator):
def __init__(self, hass: HomeAssistant, client: MyClient, config_entry: ConfigEntry) -> None:
super().__init__(
hass,
logger=LOGGER,
name=DOMAIN,
update_interval=timedelta(minutes=5),
config_entry=config_entry, # ✅ Pass config_entry - it's accepted and recommended
)
self.client = client
async def _async_update_data(self): async def _async_update_data(self):
try: try:
return await self.api.fetch_data() return await self.client.fetch_data()
except ApiError as err: except ApiError as err:
raise UpdateFailed(f"API communication error: {err}") raise UpdateFailed(f"API communication error: {err}")
``` ```
- **Error Types**: Use `UpdateFailed` for API errors, `ConfigEntryAuthFailed` for auth issues - **Error Types**: Use `UpdateFailed` for API errors, `ConfigEntryAuthFailed` for auth issues
- **Config Entry**: Always pass `config_entry` parameter to coordinator - it's accepted and recommended
## Integration Guidelines ## Integration Guidelines
@ -220,6 +237,10 @@ rules:
- Connection-critical config: Store in `ConfigEntry.data` - Connection-critical config: Store in `ConfigEntry.data`
- Non-critical settings: Store in `ConfigEntry.options` - Non-critical settings: Store in `ConfigEntry.options`
- **Validation**: Always validate user input before creating entries - **Validation**: Always validate user input before creating entries
- **Config Entry Naming**:
- ❌ Do NOT allow users to set config entry names in config flows
- Names are automatically generated or can be customized later in UI
- ✅ Exception: Helper integrations MAY allow custom names in config flow
- **Connection Testing**: Test device/service connection during config flow: - **Connection Testing**: Test device/service connection during config flow:
```python ```python
try: try:
@ -366,7 +387,8 @@ rules:
### Polling ### Polling
- Use update coordinator pattern when possible - Use update coordinator pattern when possible
- Polling intervals are NOT user-configurable - **Polling intervals are NOT user-configurable**: Never add scan_interval, update_interval, or polling frequency options to config flows or config entries
- **Integration determines intervals**: Set `update_interval` programmatically based on integration logic, not user input
- **Minimum Intervals**: - **Minimum Intervals**:
- Local network: 5 seconds - Local network: 5 seconds
- Cloud services: 60 seconds - Cloud services: 60 seconds
@ -384,6 +406,57 @@ rules:
- `ConfigEntryNotReady`: Temporary setup issues (device offline) - `ConfigEntryNotReady`: Temporary setup issues (device offline)
- `ConfigEntryAuthFailed`: Authentication problems - `ConfigEntryAuthFailed`: Authentication problems
- `ConfigEntryError`: Permanent setup issues - `ConfigEntryError`: Permanent setup issues
- **Try/Catch Best Practices**:
- Only wrap code that can throw exceptions
- Keep try blocks minimal - process data after the try/catch
- **Avoid bare exceptions** except in specific cases:
- ❌ Generally not allowed: `except:` or `except Exception:`
- ✅ Allowed in config flows to ensure robustness
- ✅ Allowed in functions/methods that run in background tasks
- Bad pattern:
```python
try:
data = await device.get_data() # Can throw
# ❌ Don't process data inside try block
processed = data.get("value", 0) * 100
self._attr_native_value = processed
except DeviceError:
_LOGGER.error("Failed to get data")
```
- Good pattern:
```python
try:
data = await device.get_data() # Can throw
except DeviceError:
_LOGGER.error("Failed to get data")
return
# ✅ Process data outside try block
processed = data.get("value", 0) * 100
self._attr_native_value = processed
```
- **Bare Exception Usage**:
```python
# ❌ Not allowed in regular code
try:
data = await device.get_data()
except Exception: # Too broad
_LOGGER.error("Failed")
# ✅ Allowed in config flow for robustness
async def async_step_user(self, user_input=None):
try:
await self._test_connection(user_input)
except Exception: # Allowed here
errors["base"] = "unknown"
# ✅ Allowed in background tasks
async def _background_refresh():
try:
await coordinator.async_refresh()
except Exception: # Allowed in task
_LOGGER.exception("Unexpected error in background task")
```
- **Setup Failure Patterns**: - **Setup Failure Patterns**:
```python ```python
try: try:
@ -445,6 +518,30 @@ rules:
- Device names - Device names
- Email addresses, usernames - Email addresses, usernames
### Entity Descriptions
- **Lambda/Anonymous Functions**: Often used in EntityDescription for value transformation
- **Multiline Lambdas**: When lambdas exceed line length, wrap in parentheses for readability
- **Bad pattern**:
```python
SensorEntityDescription(
key="temperature",
name="Temperature",
value_fn=lambda data: round(data["temp_value"] * 1.8 + 32, 1) if data.get("temp_value") is not None else None, # ❌ Too long
)
```
- **Good pattern**:
```python
SensorEntityDescription(
key="temperature",
name="Temperature",
value_fn=lambda data: ( # ✅ Parenthesis on same line as lambda
round(data["temp_value"] * 1.8 + 32, 1)
if data.get("temp_value") is not None
else None
),
)
```
### Entity Naming ### Entity Naming
- **Use has_entity_name**: Set `_attr_has_entity_name = True` - **Use has_entity_name**: Set `_attr_has_entity_name = True`
- **For specific fields**: - **For specific fields**:
@ -846,6 +943,31 @@ return {"api_key": entry.data[CONF_API_KEY]} # ❌ Exposes secrets
# Accessing hass.data directly in tests # Accessing hass.data directly in tests
coordinator = hass.data[DOMAIN][entry.entry_id] # ❌ Don't access hass.data coordinator = hass.data[DOMAIN][entry.entry_id] # ❌ Don't access hass.data
# User-configurable polling intervals
# In config flow
vol.Optional("scan_interval", default=60): cv.positive_int # ❌ Not allowed
# In coordinator
update_interval = timedelta(minutes=entry.data.get("scan_interval", 1)) # ❌ Not allowed
# User-configurable config entry names (non-helper integrations)
vol.Optional("name", default="My Device"): cv.string # ❌ Not allowed in regular integrations
# Too much code in try block
try:
response = await client.get_data() # Can throw
# ❌ Data processing should be outside try block
temperature = response["temperature"] / 10
humidity = response["humidity"]
self._attr_native_value = temperature
except ClientError:
_LOGGER.error("Failed to fetch data")
# Bare exceptions in regular code
try:
value = await sensor.read_value()
except Exception: # ❌ Too broad - catch specific exceptions
_LOGGER.error("Failed to read sensor")
``` ```
### ✅ **Use These Patterns Instead** ### ✅ **Use These Patterns Instead**
@ -875,6 +997,21 @@ return async_redact_data(data, {"api_key", "password"}) # ✅ Safe
async def init_integration(hass, mock_config_entry, mock_api): async def init_integration(hass, mock_config_entry, mock_api):
mock_config_entry.add_to_hass(hass) mock_config_entry.add_to_hass(hass)
await hass.config_entries.async_setup(mock_config_entry.entry_id) # ✅ Proper setup await hass.config_entries.async_setup(mock_config_entry.entry_id) # ✅ Proper setup
# Integration-determined polling intervals (not user-configurable)
SCAN_INTERVAL = timedelta(minutes=5) # ✅ Common pattern: constant in const.py
class MyCoordinator(DataUpdateCoordinator[MyData]):
def __init__(self, hass: HomeAssistant, client: MyClient, config_entry: ConfigEntry) -> None:
# ✅ Integration determines interval based on device capabilities, connection type, etc.
interval = timedelta(minutes=1) if client.is_local else SCAN_INTERVAL
super().__init__(
hass,
logger=LOGGER,
name=DOMAIN,
update_interval=interval,
config_entry=config_entry, # ✅ Pass config_entry - it's accepted and recommended
)
``` ```
### Entity Performance Optimization ### Entity Performance Optimization