-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
HEMS: add templates #25832
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
Draft
andig
wants to merge
4
commits into
master
Choose a base branch
from
feat/hems-tmpl
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
HEMS: add templates #25832
+286
−108
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
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:
- The generic registry type in
hems/registry/registry.gouses value receivers (func (r registry[T]) Add...) but mutatesr.data, so registrations are applied to a copy and never persist in the globalRegistry; switch these methods to pointer receivers (e.g.func (r *registry[T]) Add...) or makeRegistrya pointer to ensure entries are actually stored. - The
Types()method on the generic registry returnsmaps.Keys(r.data)without sorting, so any UI or API that consumes HEMS types will see non-deterministic ordering; consider sorting the slice before returning for stable behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The generic registry type in `hems/registry/registry.go` uses value receivers (`func (r registry[T]) Add...`) but mutates `r.data`, so registrations are applied to a copy and never persist in the global `Registry`; switch these methods to pointer receivers (e.g. `func (r *registry[T]) Add...`) or make `Registry` a pointer to ensure entries are actually stored.
- The `Types()` method on the generic registry returns `maps.Keys(r.data)` without sorting, so any UI or API that consumes HEMS types will see non-deterministic ordering; consider sorting the slice before returning for stable behavior.
## Individual Comments
### Comment 1
<location> `hems/registry/registry.go:43-45` </location>
<code_context>
+ return factory, nil
+}
+
+func (r registry[T]) Types() []string {
+ return slices.Collect(maps.Keys(r.data))
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returned type list order is non-deterministic due to map iteration.
Because map iteration order is unspecified, this will return types in a non-deterministic order. If any caller relies on stable ordering (e.g. UI dropdowns, docs, or tests), this can cause flaky behavior. To make it deterministic, sort the keys before returning, e.g.:
```go
keys := maps.Keys(r.data)
slices.Sort(keys)
return keys
```
```suggestion
func (r registry[T]) Types() []string {
keys := maps.Keys(r.data)
slices.Sort(keys)
return keys
}
```
</issue_to_address>
### Comment 2
<location> `hems/registry/registry.go:22` </location>
<code_context>
+ }
+)
+
+func (r registry[T]) Add(name string, factory func(map[string]any, site.API) (T, error)) {
+ r.AddCtx(name, func(_ context.Context, cc map[string]any, site site.API) (T, error) {
+ return factory(cc, site)
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the registry API by using a single context-aware Add method and deriving the type label from the generic type instead of passing it in manually.
You can simplify this without losing any functionality by:
1. Dropping the dual `Add` / `AddCtx` surface and standardizing on a single context‑aware factory.
2. Avoiding the extra `typ` string parameter by deriving the type name from `T`.
This keeps the generic registry but reduces API surface and call‑site requirements.
### 1. Collapse `Add` and `AddCtx` into a single method
Right now you have two registration paths:
```go
func (r registry[T]) Add(name string, factory func(map[string]any, site.API) (T, error)) {
r.AddCtx(name, func(_ context.Context, cc map[string]any, site site.API) (T, error) {
return factory(cc, site)
})
}
func (r registry[T]) AddCtx(name string, factory factoryFunc[T]) { ... }
```
You can keep all behaviour (including context‑less callers) while exposing only one method:
```go
type factoryFunc[T any] func(context.Context, map[string]any, site.API) (T, error)
type registry[T any] struct {
typ string
data map[string]factoryFunc[T]
}
func (r registry[T]) Add(name string, factory factoryFunc[T]) {
if _, exists := r.data[name]; exists {
panic(fmt.Sprintf("cannot register duplicate %s type: %s", r.typ, name))
}
r.data[name] = factory
}
```
Callers that don’t care about `context.Context` just ignore it:
```go
reg.Add("foo", func(_ context.Context, cfg map[string]any, s site.API) (hems.API, error) {
// previous body of factory(cc, site)
})
```
That removes one function, one type alias use, and the wrapping logic while preserving all capabilities.
### 2. Remove the `typ` parameter from `New` (optional but reduces coupling)
Instead of having every caller remember to pass the label:
```go
func New[T any](typ string) registry[T] {
return registry[T]{
typ: typ,
data: make(map[string]factoryFunc[T]),
}
}
```
You can derive a stable label from `T` and avoid an extra parameter:
```go
func New[T any]() registry[T] {
var zero T
return registry[T]{
typ: fmt.Sprintf("%T", zero),
data: make(map[string]factoryFunc[T]),
}
}
```
Usage stays simple:
```go
reg := registry.New[hems.API]()
```
Error messages will now be consistent and no longer rely on manually passing a string at every call site.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.