feat(registries): 扩展注册表接口功能并实现基础类#42
Merged
Merged
Conversation
- 移除 IRegistry 接口中的 in 泛型约束 - 添加 Unregister 方法用于移除指定键的项 - 添加 GetAll 方法用于获取所有键值对 - 添加 Values 方法用于获取所有注册值 - 添加 Keys 属性用于获取所有键 - 添加 Count 属性用于获取项的数量 - 在 KeyValueRegistryBase 中实现新增的接口方法 - 添加 System.Collections.ObjectModel 引用支持只读集合
Reviewer's GuideExtends the generic registry abstraction by removing contravariant constraints on the key type, enriching the IRegistry interface with querying and mutation capabilities, and providing a concrete base implementation of these new members in KeyValueRegistryBase using read‑only collection wrappers. Updated class diagram for IRegistry and KeyValueRegistryBaseclassDiagram
class IRegistry_T_Tr_ {
<<interface>>
+IEnumerable~T~ Keys
+int Count
+Tr Get(T key)
+IRegistry_T_Tr_ Registry(T key, Tr value)
+bool Unregister(T key)
+IReadOnlyDictionary~T,Tr~ GetAll()
+IReadOnlyCollection~Tr~ Values()
}
class KeyValueRegistryBase_TKey_TValue_ {
<<abstract>>
-IDictionary~TKey,TValue~ Map
+IRegistry_TKey_TValue_ Registry(TKey key, TValue value)
+IRegistry_TKey_TValue_ Registry(IKeyValue_TKey_TValue_ mapping)
+bool Unregister(TKey key)
+IReadOnlyDictionary~TKey,TValue~ GetAll()
+IReadOnlyCollection~TValue~ Values()
+IEnumerable~TKey~ Keys
+int Count
}
class IKeyValue_TKey_TValue_ {
<<interface>>
+TKey Key
+TValue Value
}
KeyValueRegistryBase_TKey_TValue_ ..|> IRegistry_TKey_TValue_
KeyValueRegistryBase_TKey_TValue_ ..> IKeyValue_TKey_TValue_
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
KeyValueRegistryBase.Values(), the fallback branch createsnew ReadOnlyCollection<TValue>([]), which always returns an empty collection instead of wrappingMap.Values; you likely want to materialize fromMap.Values(e.g.,new ReadOnlyCollection<TValue>(Map.Values.ToList())) so the method actually reflects the registry contents. GetAll()currently returns aReadOnlyDictionarywrapper over the underlyingMap, which exposes a live view rather than a snapshot; consider clarifying this behavior in the method name or returning a copy (e.g.,new ReadOnlyDictionary<TKey, TValue>(new Dictionary<TKey, TValue>(Map))) if callers should not see future mutations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `KeyValueRegistryBase.Values()`, the fallback branch creates `new ReadOnlyCollection<TValue>([])`, which always returns an empty collection instead of wrapping `Map.Values`; you likely want to materialize from `Map.Values` (e.g., `new ReadOnlyCollection<TValue>(Map.Values.ToList())`) so the method actually reflects the registry contents.
- `GetAll()` currently returns a `ReadOnlyDictionary` wrapper over the underlying `Map`, which exposes a live view rather than a snapshot; consider clarifying this behavior in the method name or returning a copy (e.g., `new ReadOnlyDictionary<TKey, TValue>(new Dictionary<TKey, TValue>(Map))`) if callers should not see future mutations.
## Individual Comments
### Comment 1
<location> `GFramework.Core.Abstractions/registries/KeyValueRegistryBase.cs:99-101` </location>
<code_context>
+ /// 获取注册表中所有的值
+ /// </summary>
+ /// <returns>包含所有注册值的只读集合</returns>
+ public IReadOnlyCollection<TValue> Values()
+ {
+ return Map.Values as IReadOnlyCollection<TValue> ?? new ReadOnlyCollection<TValue>([]);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The fallback branch returns an empty collection instead of the actual registry values.
`new ReadOnlyCollection<TValue>([])` always returns an empty collection, discarding the contents of `Map`. If the cast fails, you likely want a read-only view over the actual values, e.g. `new ReadOnlyCollection<TValue>(Map.Values.ToList())`, so the result still reflects `Map`’s current values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- 修复了当 Map.Values 无法转换为 IReadOnlyCollection<TValue> 时返回空集合的问题 - 现在正确地将 Map.Values 转换为 List 后再创建 ReadOnlyCollection - 确保 Values 方法始终返回包含实际数据的集合而不是空集合
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
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.
Summary by Sourcery
Extend the registry abstraction with richer query and management capabilities and implement them in the base key-value registry.
New Features:
Enhancements: