-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Vehicles: remove title/icon from yaml config (BC) #24258
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: master
Are you sure you want to change the base?
Conversation
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.
Hey there - I've reviewed your changes - here's some feedback:
- There’s a lot of repetitive withContext(ctx) boilerplate in each vehicle constructor—consider refactoring the registry.AddCtx wrapper to automatically inject context into embeds and reduce duplication.
- Instead of leaving large blocks of commented-out code (e.g. legacy site.Vehicles and Icon/SetTitle methods), remove them entirely to clean up the codebase and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repetitive withContext(ctx) boilerplate in each vehicle constructor—consider refactoring the registry.AddCtx wrapper to automatically inject context into embeds and reduce duplication.
- Instead of leaving large blocks of commented-out code (e.g. legacy site.Vehicles and Icon/SetTitle methods), remove them entirely to clean up the codebase and avoid confusion.
## Individual Comments
### Comment 1
<location> `core/site_vehicles.go:87` </location>
<code_context>
}
-var _ site.Vehicles = (*vehicles)(nil)
+// var _ site.Vehicles = (*vehicles)(nil)
-type vehicles struct {
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the commented-out code to improve code clarity and maintainability.
</issue_to_address>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andig any chance we can keep supporting the deprecated yaml title/icon for a couple of releases? Since config ui is not marked as stable yet, there will be no non-experimental way to set a vehicle title. Having a vehicle title is much more critical from a user-base-perspective than meter titles. If we can't support both, I'd say we should hold this PR until config ui is stable. |
Is the new vehicle title api really necessary for this? Publishing a vehicle |
This PR removes the special case of vehicle having their "own" title and icon instead of title/icon being part of the device meta data. This has the following consequences:
TODO