support validator null stub db and config access#730
support validator null stub db and config access#730zouyonghe merged 3 commits intoAstrBotDevs:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the NullStub class in scripts/validate_plugins/run.py by implementing asynchronous context manager methods (__aenter__, __aexit__) and dictionary-like methods (get, pop). It also includes new test cases in tests/test_validate_plugins.py to ensure these methods function correctly within async database context patterns and configuration access. The review feedback recommends improving code idiomaticity by using underscores for unused arguments instead of del and adding type hints for better maintainability.
| async def __aexit__(self, exc_type, exc, tb) -> bool: | ||
| del exc_type, exc, tb | ||
| return False |
There was a problem hiding this comment.
For better code quality and maintainability, it's good to add type hints to arguments. Also, for unused arguments, it's more idiomatic in Python to prefix them with an underscore (_) rather than using del, as recommended by style guides like PEP 8. Since types from the types module are not imported, we can use object as a general type for now.
| async def __aexit__(self, exc_type, exc, tb) -> bool: | |
| del exc_type, exc, tb | |
| return False | |
| async def __aexit__(self, _exc_type: object, _exc: object, _tb: object) -> bool: | |
| return False |
| def get(self, key=None, default=None): | ||
| del key | ||
| return default |
There was a problem hiding this comment.
For better code quality and maintainability, it's good to add type hints. Also, for unused arguments, it's more idiomatic in Python to prefix them with an underscore (_) rather than using del, as recommended by style guides like PEP 8. Since typing.Any is not imported, we can use object as a general type for the arguments and return value.
| def get(self, key=None, default=None): | |
| del key | |
| return default | |
| def get(self, _key: object = None, default: object = None) -> object: | |
| return default |
| def pop(self, key=None, default=None): | ||
| del key | ||
| return default |
There was a problem hiding this comment.
For better code quality and maintainability, it's good to add type hints. Also, for unused arguments, it's more idiomatic in Python to prefix them with an underscore (_) rather than using del, as recommended by style guides like PEP 8. Since typing.Any is not imported, we can use object as a general type for the arguments and return value.
| def pop(self, key=None, default=None): | |
| del key | |
| return default | |
| def pop(self, _key: object = None, default: object = None) -> object: | |
| return default |
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- DummyContext eagerly creates the data and plugin_data directories in init and again in get_data_dir; consider deferring directory creation to get_data_dir (or a shared helper) to avoid duplicated logic and unnecessary filesystem side effects when a context is instantiated but data access is not needed.
- NullStub.get and NullStub.pop currently ignore the key argument entirely; if any validation paths rely on inspecting requested keys, you may want to at least record or expose the requested key (or accept **kwargs to better mirror dict.get/pop signatures) while still returning the provided default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- DummyContext eagerly creates the data and plugin_data directories in __init__ and again in get_data_dir; consider deferring directory creation to get_data_dir (or a shared helper) to avoid duplicated logic and unnecessary filesystem side effects when a context is instantiated but data access is not needed.
- NullStub.get and NullStub.pop currently ignore the key argument entirely; if any validation paths rely on inspecting requested keys, you may want to at least record or expose the requested key (or accept **kwargs to better mirror dict.get/pop signatures) while still returning the provided default.
## Individual Comments
### Comment 1
<location path="scripts/validate_plugins/run.py" line_range="655" />
<code_context>
return False
+class DummyConfig(dict):
+ def __init__(self, initial=None) -> None:
+ super().__init__()
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying DummyConfig’s special behaviors and making DummyContext’s config/path accessors more explicit and side‑effect free to reduce hidden magic.
You can keep the new behavior but reduce the “magic” in two focused ways:
---
### 1. Simplify `DummyConfig` and make behavior explicit
Right now `DummyConfig`:
- Overrides `__setitem__`, `__getitem__`, and `__getattr__`
- Recursively wraps nested dicts
- Changes standard dict semantics (`__getitem__` returns `NullStub` instead of raising)
You can keep “missing keys → `NullStub`” while:
- Restoring normal `.get()` behavior
- Avoiding recursive wrapping
- Restricting the magic to the minimum surface area
Example refactor:
```python
class DummyConfig(dict):
def __init__(self, initial=None) -> None:
super().__init__(initial or {})
def __missing__(self, key):
# Only affect direct key access; .get() stays standard
return NullStub()
def __getattr__(self, name: str):
# Attribute access uses the same semantics as key access
try:
return self[name]
except KeyError:
return NullStub()
```
If you still need nested dicts to behave like `DummyConfig`, you can wrap only the specific nested sections you care about at construction time, instead of recursively wrapping everything:
```python
self._config = DummyConfig(
{
"wake_prefix": [],
"dashboard": DummyConfig(),
"admins_id": [],
"admin_ids": [],
"platform_settings": DummyConfig(
{
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
}
),
"data_dir": str(self._data_root),
}
)
```
This keeps the “feels like config object, missing → `NullStub`” behavior but avoids overriding `__getitem__` and avoids fully dynamic recursive wrapping.
---
### 2. Make `DummyContext` config/path handling more explicit and side‑effect free
`DummyContext.__init__` is doing setup correctly, but:
- `get_config` and `get_context_config` are aliases with no added value
- `get_data_dir` does another `mkdir` on every call
You can:
- Initialize directories once in `__init__`
- Have accessors be pure getters
- Optionally keep both config methods as aliases but make that obvious and side‑effect free
Example:
```python
class DummyContext:
def __init__(self) -> None:
self._star_manager = None
self._astrbot_root = Path(os.environ.get("ASTRBOT_ROOT", Path.cwd())).resolve()
self._data_root = self._astrbot_root / "data"
self._plugin_data_dir = self._data_root / "plugin_data"
self._plugin_data_dir.mkdir(parents=True, exist_ok=True)
self._config = DummyConfig(
{
"wake_prefix": [],
"dashboard": {},
"admins_id": [],
"admin_ids": [],
"platform_settings": {
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
},
"data_dir": str(self._data_root),
}
)
self.config = self._config # public alias if needed
def get_config(self, umo: str | None = None):
del umo
return self._config
# If you must keep this for compatibility, delegate explicitly
def get_context_config(self):
return self.get_config()
def get_data_dir(self) -> str:
# No side effects; directory already ensured in __init__
return str(self._plugin_data_dir)
```
This preserves the current interfaces (`config`, `get_config`, `get_context_config`, `get_data_dir`) and behavior (paths, config content, `NullStub` semantics) while making the behavior more explicit and reducing hidden side effects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return False | ||
|
|
||
|
|
||
| class DummyConfig(dict): |
There was a problem hiding this comment.
issue (complexity): Consider simplifying DummyConfig’s special behaviors and making DummyContext’s config/path accessors more explicit and side‑effect free to reduce hidden magic.
You can keep the new behavior but reduce the “magic” in two focused ways:
1. Simplify DummyConfig and make behavior explicit
Right now DummyConfig:
- Overrides
__setitem__,__getitem__, and__getattr__ - Recursively wraps nested dicts
- Changes standard dict semantics (
__getitem__returnsNullStubinstead of raising)
You can keep “missing keys → NullStub” while:
- Restoring normal
.get()behavior - Avoiding recursive wrapping
- Restricting the magic to the minimum surface area
Example refactor:
class DummyConfig(dict):
def __init__(self, initial=None) -> None:
super().__init__(initial or {})
def __missing__(self, key):
# Only affect direct key access; .get() stays standard
return NullStub()
def __getattr__(self, name: str):
# Attribute access uses the same semantics as key access
try:
return self[name]
except KeyError:
return NullStub()If you still need nested dicts to behave like DummyConfig, you can wrap only the specific nested sections you care about at construction time, instead of recursively wrapping everything:
self._config = DummyConfig(
{
"wake_prefix": [],
"dashboard": DummyConfig(),
"admins_id": [],
"admin_ids": [],
"platform_settings": DummyConfig(
{
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
}
),
"data_dir": str(self._data_root),
}
)This keeps the “feels like config object, missing → NullStub” behavior but avoids overriding __getitem__ and avoids fully dynamic recursive wrapping.
2. Make DummyContext config/path handling more explicit and side‑effect free
DummyContext.__init__ is doing setup correctly, but:
get_configandget_context_configare aliases with no added valueget_data_dirdoes anothermkdiron every call
You can:
- Initialize directories once in
__init__ - Have accessors be pure getters
- Optionally keep both config methods as aliases but make that obvious and side‑effect free
Example:
class DummyContext:
def __init__(self) -> None:
self._star_manager = None
self._astrbot_root = Path(os.environ.get("ASTRBOT_ROOT", Path.cwd())).resolve()
self._data_root = self._astrbot_root / "data"
self._plugin_data_dir = self._data_root / "plugin_data"
self._plugin_data_dir.mkdir(parents=True, exist_ok=True)
self._config = DummyConfig(
{
"wake_prefix": [],
"dashboard": {},
"admins_id": [],
"admin_ids": [],
"platform_settings": {
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
},
"data_dir": str(self._data_root),
}
)
self.config = self._config # public alias if needed
def get_config(self, umo: str | None = None):
del umo
return self._config
# If you must keep this for compatibility, delegate explicitly
def get_context_config(self):
return self.get_config()
def get_data_dir(self) -> str:
# No side effects; directory already ensured in __init__
return str(self._plugin_data_dir)This preserves the current interfaces (config, get_config, get_context_config, get_data_dir) and behavior (paths, config content, NullStub semantics) while making the behavior more explicit and reducing hidden side effects.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/validate_plugins/run.py" line_range="655" />
<code_context>
return False
+class DummyConfig(dict):
+ def __init__(self, initial=None) -> None:
+ super().__init__(initial or {})
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new DummyConfig and DummyContext setup by using defaultdict and a single config attribute to reduce indirection and magic behavior.
You can keep the new functionality but simplify some of the indirection and “magic”, mainly around `DummyConfig` and the duplicated config attributes.
### 1. Simplify `DummyConfig` with `defaultdict`
You can get the same behavior (auto-`NullStub()` for missing keys + attribute access) without a custom `__missing__` by using `defaultdict`. This keeps the behavior explicit and easier to follow:
```python
from collections import defaultdict
class DummyConfig(defaultdict):
def __init__(self, initial=None) -> None:
super().__init__(lambda: NullStub(), initial or {})
def __getattr__(self, name: str):
# preserves attribute access: config.foo == config["foo"]
return self[name]
```
This preserves:
- `config["missing"]` → `NullStub()`
- `config.missing` → `NullStub()`
- existing keys behaving as-is
while removing the need for a custom `__missing__` implementation.
### 2. Drop redundant `_config` vs `config`
`DummyContext` currently stores the same config in `_config` and `config`, and `get_config` returns `_config`. You can keep behavior but remove the duplication:
```python
class DummyContext:
def __init__(self) -> None:
self._star_manager = None
self._astrbot_root = Path(os.environ.get("ASTRBOT_ROOT", Path.cwd())).resolve()
self._data_root = self._astrbot_root / "data"
self._plugin_data_dir = self._data_root / "plugin_data"
self.config = DummyConfig(
{
"wake_prefix": [],
"dashboard": DummyConfig(),
"admins_id": [],
"admin_ids": [],
"platform_settings": DummyConfig(
{
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
}
),
"data_dir": str(self._data_root),
}
)
def get_config(self, umo: str | None = None):
del umo
return self.config
def get_context_config(self):
return self.get_config()
```
This keeps the same external API (`config` attribute, `get_config`, `get_context_config`) but reduces internal indirection.
These two changes reduce the “magic” and surface area without changing the semantics your tests/plugins see.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return False | ||
|
|
||
|
|
||
| class DummyConfig(dict): |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the new DummyConfig and DummyContext setup by using defaultdict and a single config attribute to reduce indirection and magic behavior.
You can keep the new functionality but simplify some of the indirection and “magic”, mainly around DummyConfig and the duplicated config attributes.
1. Simplify DummyConfig with defaultdict
You can get the same behavior (auto-NullStub() for missing keys + attribute access) without a custom __missing__ by using defaultdict. This keeps the behavior explicit and easier to follow:
from collections import defaultdict
class DummyConfig(defaultdict):
def __init__(self, initial=None) -> None:
super().__init__(lambda: NullStub(), initial or {})
def __getattr__(self, name: str):
# preserves attribute access: config.foo == config["foo"]
return self[name]This preserves:
config["missing"]→NullStub()config.missing→NullStub()- existing keys behaving as-is
while removing the need for a custom __missing__ implementation.
2. Drop redundant _config vs config
DummyContext currently stores the same config in _config and config, and get_config returns _config. You can keep behavior but remove the duplication:
class DummyContext:
def __init__(self) -> None:
self._star_manager = None
self._astrbot_root = Path(os.environ.get("ASTRBOT_ROOT", Path.cwd())).resolve()
self._data_root = self._astrbot_root / "data"
self._plugin_data_dir = self._data_root / "plugin_data"
self.config = DummyConfig(
{
"wake_prefix": [],
"dashboard": DummyConfig(),
"admins_id": [],
"admin_ids": [],
"platform_settings": DummyConfig(
{
"aiocqhttp": {},
"qqofficial": {},
"telegram": {},
"gewechat": {},
"wechatpadpro": {},
}
),
"data_dir": str(self._data_root),
}
)
def get_config(self, umo: str | None = None):
del umo
return self.config
def get_context_config(self):
return self.get_config()This keeps the same external API (config attribute, get_config, get_context_config) but reduces internal indirection.
These two changes reduce the “magic” and surface area without changing the semantics your tests/plugins see.
1 participant
Summary
NullStubto behave like an async context manager for database-style access patternsNullStub.get(...)andNullStub.pop(...)so config fallback code can load without spuriousNullStubtype errorsWhy
The previous validator stub reported false negatives for plugins that legitimately rely on runtime patterns such as:
async with context.get_db().get_db() as sessioncontext.get_config().get("dashboard", {}).get("port", 6185)Those plugins were failing because the validator stub was less capable than the real AstrBot runtime, not because the plugin logic itself was necessarily broken.
Testing
python3 -m unittest tests.test_validate_plugins tests.test_detect_changed_pluginsSummary by Sourcery
Extend the validator dummy context and null stub to better emulate runtime database and configuration access patterns used by plugins.
New Features:
Tests: