Refactor CacheManager to use generic type T #87
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
审阅者指南（在小型 PR 上折叠）审阅者指南将缓存系统重构为完全对类型 T 泛型化，确保 Cache 和 CacheManager 之间的类型一致性，而不是使用 BaseData/Any。 泛型 Cache 与 CacheManager 的类图classDiagram
class Cache~T~ {
+int max_size
+OrderedDict~str, T~ _cache
+__post_init__()
}
class CacheManager~T~ {
<<singleton>>
-CacheManager _instance
-dict~CacheCategoryEnum, Cache~T~~ _cached
+__new__() Self
+get_cache(category CacheCategoryEnum, max_size int) Cache~T~
}
CacheManager~T~ "1" --> "*" Cache~T~ : manages
class CacheCategoryEnum {
}
class OrderedDict~K, V~ {
}
文件级变更
技巧与命令与 Sourcery 交互
自定义你的体验访问你的控制面板 以：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the cache system to be fully generic over type T, ensuring type consistency between Cache and CacheManager instead of using BaseData/Any. Class diagram for generic Cache and CacheManagerclassDiagram
class Cache~T~ {
+int max_size
+OrderedDict~str, T~ _cache
+__post_init__()
}
class CacheManager~T~ {
<<singleton>>
-CacheManager _instance
-dict~CacheCategoryEnum, Cache~T~~ _cached
+__new__() Self
+get_cache(category CacheCategoryEnum, max_size int) Cache~T~
}
CacheManager~T~ "1" --> "*" Cache~T~ : manages
class CacheCategoryEnum {
}
class OrderedDict~K, V~ {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体性的反馈：
- 在仍然使用单个
_instance单例的情况下将CacheManager变为Generic[T]，意味着所有类型参数共享同一个底层缓存实例，这会让类型检查变得混乱，并可能混用不同的T用法；建议要么从这个单例中移除泛型，要么让单例与类型相关（针对不同T拥有不同实例）。 - 如果
Cache预期只保存类似BaseData的对象，把_cache更新为OrderedDict[str, T]但又没有约束T（例如TypeVar('T', bound=BaseData)），就削弱了类型安全性；建议为T添加合适的上界约束，以保持原有契约。
供 AI 代理使用的提示词
请根据以下代码审查意见进行修改：
## 整体意见
- 在仍然使用单个 `_instance` 单例的情况下将 `CacheManager` 变为 `Generic[T]`，意味着所有类型参数共享同一个底层缓存实例，这会让类型检查变得混乱，并可能混用不同的 `T` 用法；建议要么从这个单例中移除泛型，要么让单例与类型相关（针对不同 `T` 拥有不同实例）。
- 如果 `Cache` 预期只保存类似 `BaseData` 的对象，把 `_cache` 更新为 `OrderedDict[str, T]` 但又没有约束 `T`（例如 `TypeVar('T', bound=BaseData)`），就削弱了类型安全性；建议为 `T` 添加合适的上界约束，以保持原有契约。
## 单条评论
### 评论 1
<location> `nonebot_plugin_value/_cache.py:87-89` </location>
<code_context>
-class CacheManager:
+class CacheManager(Generic[T]):
_instance = None
- _cached: dict[CacheCategoryEnum, Cache[Any]]
+ _cached: dict[CacheCategoryEnum, Cache[T]]
def __new__(cls) -> Self:
</code_context>
<issue_to_address>
**issue (bug_risk)：** 在单例管理器上使用 `Generic[T]`，在用不同类型参数实例化时会导致令人困惑或不健全的类型行为。
由于 `_instance` 是共享的，`CacheManager[int]()` 和 `CacheManager[str]()` 实际上引用的是同一个对象，但在类型检查器看来却是不同的泛型类型。这会使 `_cached` 和 `get_cache` 变得类型不安全。如果你只需要一个全局管理器，请从 `CacheManager` 中移除 `Generic[T]`，只让 `Cache` 保持泛型。如果你确实需要按 `T` 区分的管理器，请让 `_instance` 与类型相关（例如针对每个 `T` 拥有一个单独的单例实例）。
</issue_to_address>
### 评论 2
<location> `nonebot_plugin_value/_cache.py:97-99` </location>
<code_context>
async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
- ) -> Cache[Any]:
+ ) -> Cache[T]:
# 为不同类别创建具有不同大小的缓存
if category not in self._cached:
</code_context>
<issue_to_address>
**suggestion：** `get_cache` 的泛型返回类型可能无法体现不同分类之间缓存内容的异质性。
在这里使用 `Cache[T]` 意味着所有 `CacheCategoryEnum` 的值都使用相同的 `T`。如果不同分类存放的值类型不同，在重用 `CacheManager` 时，这就会变得具有误导性并且类型不安全。根据实际使用情况，可以考虑对 `CacheManager` 使用非泛型的 `Cache[Any]`，或者引入一个显式的“分类→类型”映射（例如通过 `Protocol` 或辅助工具），从而让不同缓存类型在类型检查器中是可区分的。
建议实现：
```python
class CacheManager:
_instance = None
_cached: dict[CacheCategoryEnum, Cache[Any]]
```
```python
async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
) -> Cache[Any]:
```
1. 从该模块中移除未使用的泛型类型参数 `T`：
- 如果 `T` 只用于 `CacheManager`，则删除相应的 `TypeVar` 声明。
- 如果没有其他类使用 `Generic`，则从导入中删除 `Generic`。
2. 如果目前有调用方使用了 `CacheManager[T]` 的注解或期望一个泛型 `CacheManager`，请更新它们以不带类型参数地引用 `CacheManager`。
3. 确保已经从 `typing` 中导入了 `Any`（`from typing import Any`），如果尚未导入的话。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的审查。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Turning
CacheManagerintoGeneric[T]while still using a single_instancesingleton means all type parameters share the same underlying cache instance, which will confuse type checking and may mix differentTusages; consider either removing the generic from the singleton or making the singleton type-specific. - If
Cacheis intended to hold onlyBaseData-like objects, updating_cachetoOrderedDict[str, T]without constrainingT(e.g.,TypeVar('T', bound=BaseData)) weakens type safety; consider adding an appropriate bound onTto preserve the original contract.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Turning `CacheManager` into `Generic[T]` while still using a single `_instance` singleton means all type parameters share the same underlying cache instance, which will confuse type checking and may mix different `T` usages; consider either removing the generic from the singleton or making the singleton type-specific.
- If `Cache` is intended to hold only `BaseData`-like objects, updating `_cache` to `OrderedDict[str, T]` without constraining `T` (e.g., `TypeVar('T', bound=BaseData)`) weakens type safety; consider adding an appropriate bound on `T` to preserve the original contract.
## Individual Comments
### Comment 1
<location> `nonebot_plugin_value/_cache.py:87-89` </location>
<code_context>
-class CacheManager:
+class CacheManager(Generic[T]):
_instance = None
- _cached: dict[CacheCategoryEnum, Cache[Any]]
+ _cached: dict[CacheCategoryEnum, Cache[T]]
def __new__(cls) -> Self:
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Generic[T]` on a singleton manager can lead to confusing or unsound typing when instantiated with different type parameters.
Since `_instance` is shared, `CacheManager[int]()` and `CacheManager[str]()` refer to the same object but appear as different generic types to the type checker. This can make `_cached` and `get_cache` type-unsafe. If you only need one global manager, drop `Generic[T]` from `CacheManager` and keep only `Cache` generic. If you truly need per-`T` managers, make `_instance` type-specific (e.g., one singleton instance per `T`).
</issue_to_address>
### Comment 2
<location> `nonebot_plugin_value/_cache.py:97-99` </location>
<code_context>
async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
- ) -> Cache[Any]:
+ ) -> Cache[T]:
# 为不同类别创建具有不同大小的缓存
if category not in self._cached:
</code_context>
<issue_to_address>
**suggestion:** The generic return type of `get_cache` might not reflect per-category heterogeneity of cache contents.
Using `Cache[T]` here implies all `CacheCategoryEnum` values use the same `T`. If categories hold different value types, this becomes misleading and type-unsafe as `CacheManager` is reused. Depending on actual usage, consider either using a non-generic `Cache[Any]` for `CacheManager`, or introducing an explicit category→type mapping (e.g. via a `Protocol` or helper) so different cache types are distinguishable to the type checker.
Suggested implementation:
```python
class CacheManager:
_instance = None
_cached: dict[CacheCategoryEnum, Cache[Any]]
```
```python
async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
) -> Cache[Any]:
```
1. Remove the unused generic type parameter `T` from this module:
- Drop `T` from any `TypeVar` declaration if it is only used for `CacheManager`.
- Remove `Generic` from the imports if no other class uses it.
2. If any callers currently annotate `CacheManager[T]` or expect a generic `CacheManager`, update them to reference `CacheManager` without type parameters.
3. Ensure `Any` is imported from `typing` if it is not already (`from typing import Any`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+87
to
+89
| class CacheManager(Generic[T]): | ||
| _instance = None | ||
| _cached: dict[CacheCategoryEnum, Cache[Any]] | ||
| _cached: dict[CacheCategoryEnum, Cache[T]] |
Contributor
There was a problem hiding this comment.
issue (bug_risk)： 在单例管理器上使用 Generic[T]，在用不同类型参数实例化时会导致令人困惑或不健全的类型行为。
由于 _instance 是共享的，CacheManager[int]() 和 CacheManager[str]() 实际上引用的是同一个对象，但在类型检查器看来却是不同的泛型类型。这会使 _cached 和 get_cache 变得类型不安全。如果你只需要一个全局管理器，请从 CacheManager 中移除 Generic[T]，只让 Cache 保持泛型。如果你确实需要按 T 区分的管理器，请让 _instance 与类型相关（例如针对每个 T 拥有一个单独的单例实例）。
Original comment in English
issue (bug_risk): Using Generic[T] on a singleton manager can lead to confusing or unsound typing when instantiated with different type parameters.
Since _instance is shared, CacheManager[int]() and CacheManager[str]() refer to the same object but appear as different generic types to the type checker. This can make _cached and get_cache type-unsafe. If you only need one global manager, drop Generic[T] from CacheManager and keep only Cache generic. If you truly need per-T managers, make _instance type-specific (e.g., one singleton instance per T).
Comment on lines
97
to
+99
| async def get_cache( | ||
| self, category: CacheCategoryEnum, max_size: int = 1000 | ||
| ) -> Cache[Any]: | ||
| ) -> Cache[T]: |
Contributor
There was a problem hiding this comment.
suggestion： get_cache 的泛型返回类型可能无法体现不同分类之间缓存内容的异质性。
在这里使用 Cache[T] 意味着所有 CacheCategoryEnum 的值都使用相同的 T。如果不同分类存放的值类型不同，在重用 CacheManager 时，这就会变得具有误导性并且类型不安全。根据实际使用情况，可以考虑对 CacheManager 使用非泛型的 Cache[Any]，或者引入一个显式的“分类→类型”映射（例如通过 Protocol 或辅助工具），从而让不同缓存类型在类型检查器中是可区分的。
建议实现：
class CacheManager:
_instance = None
_cached: dict[CacheCategoryEnum, Cache[Any]] async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
) -> Cache[Any]:- 从该模块中移除未使用的泛型类型参数
T：- 如果
T只用于CacheManager，则删除相应的TypeVar声明。 - 如果没有其他类使用
Generic，则从导入中删除Generic。
- 如果
- 如果目前有调用方使用了
CacheManager[T]的注解或期望一个泛型CacheManager，请更新它们以不带类型参数地引用CacheManager。 - 确保已经从
typing中导入了Any（from typing import Any），如果尚未导入的话。
Original comment in English
suggestion: The generic return type of get_cache might not reflect per-category heterogeneity of cache contents.
Using Cache[T] here implies all CacheCategoryEnum values use the same T. If categories hold different value types, this becomes misleading and type-unsafe as CacheManager is reused. Depending on actual usage, consider either using a non-generic Cache[Any] for CacheManager, or introducing an explicit category→type mapping (e.g. via a Protocol or helper) so different cache types are distinguishable to the type checker.
Suggested implementation:
class CacheManager:
_instance = None
_cached: dict[CacheCategoryEnum, Cache[Any]] async def get_cache(
self, category: CacheCategoryEnum, max_size: int = 1000
) -> Cache[Any]:- Remove the unused generic type parameter
Tfrom this module:- Drop
Tfrom anyTypeVardeclaration if it is only used forCacheManager. - Remove
Genericfrom the imports if no other class uses it.
- Drop
- If any callers currently annotate
CacheManager[T]or expect a genericCacheManager, update them to referenceCacheManagerwithout type parameters. - Ensure
Anyis imported fromtypingif it is not already (from typing import Any).
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
重构缓存系统，使
Cache和CacheManager在值类型T上实现完全泛型，以获得更严格的类型安全性。增强内容：
Cache._cache存储泛型类型T的值，而不是BaseData。T参数化CacheManager，并在其内部缓存映射和get_cache方法中传递T。Original summary in English
Summary by Sourcery
Refactor the cache system to make Cache and CacheManager fully generic over value type T for stricter type safety.
Enhancements: