Bugfix: Fix drivesomethinggreater related vehicle templates#31047
Bugfix: Fix drivesomethinggreater related vehicle templates#31047kaindl wants to merge 5 commits into
Conversation
Included description how to set up the reqiurements in the dsg-portal in all templates include params for generic features directly / via preset "vehicle-generic-online-functions", as preset "vehicle-features" should not be used together with mandatory features
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
requirements.descriptionblock for the EU Data Act portal is duplicated verbatim across multiple brand templates; consider centralizing this in a shared include/preset or a single source to keep future changes consistent. - The
coarsecurrentandwelcomechargeparams are now repeated in several templates; if these are always used together, it might be cleaner to introduce or extend a preset (e.g., alongsidevehicle-generic-online-functions) instead of defining them in each file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `requirements.description` block for the EU Data Act portal is duplicated verbatim across multiple brand templates; consider centralizing this in a shared include/preset or a single source to keep future changes consistent.
- The `coarsecurrent` and `welcomecharge` params are now repeated in several templates; if these are always used together, it might be cleaner to introduce or extend a preset (e.g., alongside `vehicle-generic-online-functions`) instead of defining them in each file.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The repeated
requirements.descriptionblock for the drivesomethinggreater portal across multiple templates (vw, audi, seat-cupra) could be factored into a shared preset or YAML anchor to avoid duplication and keep future updates consistent. - The
coarsecurrentandwelcomechargeparams are now repeated in several vehicle templates; consider introducing a dedicated preset for these generic online features to keep parameter definitions DRY and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `requirements.description` block for the drivesomethinggreater portal across multiple templates (vw, audi, seat-cupra) could be factored into a shared preset or YAML anchor to avoid duplication and keep future updates consistent.
- The `coarsecurrent` and `welcomecharge` params are now repeated in several vehicle templates; consider introducing a dedicated preset for these generic online features to keep parameter definitions DRY and easier to maintain.
## Individual Comments
### Comment 1
<location path="templates/definition/vehicle/audi.yaml" line_range="7-16" />
<code_context>
+requirements:
</code_context>
<issue_to_address>
**suggestion:** Consider deduplicating the repeated EU Data Act requirements text across brand templates.
The de/en `requirements.description` block here matches the ones in `vw.yaml` and `seat-cupra.yaml`. To avoid future drift when this text changes, consider moving it to a shared include or common preset and referencing it from each brand template, if the templating system supports that.
Suggested implementation:
```
- brand: Audi
description:
generic: EU Data Act
requirements: *eu_data_act_requirements
```
1. Define a shared YAML anchor (e.g. `&eu_data_act_requirements`) containing the common `requirements.description` (both `de` and `en`) in a common location that is loaded for all brand templates (for example, a `templates/definition/vehicle/common.yaml` or similar).
2. In `vw.yaml` and `seat-cupra.yaml`, replace their duplicated `requirements` blocks with `requirements: *eu_data_act_requirements` (or the appropriate alias name) to reuse the shared text.
3. Ensure your templating/build system actually loads the file that defines `&eu_data_act_requirements` before/alongside `audi.yaml`, `vw.yaml`, and `seat-cupra.yaml`, since YAML anchors do not work across completely independent parse trees unless your tooling merges them first.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - preset: vehicle-base | ||
| - name: vin | ||
| example: WAUZZZ... | ||
| - preset: vehicle-features |
There was a problem hiding this comment.
As already answered to your first questionmark:
vehicle-features contains "streaming", but streaming is set as mandatory in these templates. I think it is confusing for users to have a parameter in the gui, that is on even when you switched it off...
There was a problem hiding this comment.
@naltatis could we suppress rendering params that have a non-zero default value? These can‘t be deselected anyway. Maybe limited to bool obly?
There was a problem hiding this comment.
could we suppress rendering params that have a non-zero default value? These can‘t be deselected anyway. Maybe limited to bool obly?
Sounds like something that has potential to create confusion in the future. I'd rather keep it simple/deterministic - this would also affect the rendered docs.
Breaking up (inlining) the preset seems like the right move here. I'd rather reorganize the presets in the future, than implementing revert/hide preset params from within the template.
There was a problem hiding this comment.
The problem with breaking up the presets is that you'll need to do this everywhere every time a single preset is added.
|
@andig are coarsecurrent and welcomecharge unneeded for cars of all VW brands and platforms? I would guess so, as apart from eUp-Derivates all platforms I know, that are in the VW connect system, are also used by Audi.. |
|
Afaia yes |
…udi, Drivesomethinggreater, Seat-Cupra, and VW templates
…rify data provision
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
requirements.descriptionblocks for VW, Audi, and SEAT/CUPRA are largely duplicated; consider extracting the common EU Data Act / portal setup text into a shared snippet or preset to avoid future drift between brands. - You switched from
vehicle-featurestovehicle-onlinein these DSG-related templates; if there are other drivesomethinggreater vehicle templates still usingvehicle-features, it may be worth aligning them for consistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `requirements.description` blocks for VW, Audi, and SEAT/CUPRA are largely duplicated; consider extracting the common EU Data Act / portal setup text into a shared snippet or preset to avoid future drift between brands.
- You switched from `vehicle-features` to `vehicle-online` in these DSG-related templates; if there are other drivesomethinggreater vehicle templates still using `vehicle-features`, it may be worth aligning them for consistent behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
depends on #31046