Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions scripts/validate_plugins/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -630,16 +630,69 @@ async def _return_self():

return _return_self().__await__()

async def __aenter__(self) -> "NullStub":
return self

async def __aexit__(self, exc_type, exc, tb) -> bool:
del exc_type, exc, tb
return False
Comment on lines +636 to +638
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
Comment on lines +640 to +642
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
Comment on lines +644 to +646
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
def pop(self, key=None, default=None):
del key
return default
def pop(self, _key: object = None, default: object = None) -> object:
return default


def __iter__(self):
return iter(())

def __bool__(self) -> bool:
return False


class DummyConfig(dict):
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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_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:

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.missingNullStub()
  • 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.

def __init__(self, initial=None) -> None:
super().__init__(initial or {})

def __missing__(self, key):
del key
return NullStub()

def __getattr__(self, name: str):
return self[name]


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),
}
)
self.config = self._config

def _ensure_plugin_data_dir(self) -> Path:
self._plugin_data_dir.mkdir(parents=True, exist_ok=True)
return self._plugin_data_dir

def get_all_stars(self):
try:
Expand Down Expand Up @@ -669,6 +722,16 @@ def register_llm_tool(self, name: str, func_args, desc: str, func_obj) -> None:
def unregister_llm_tool(self, name: str) -> None:
del name

def get_config(self, umo: str | None = None):
del umo
return self._config

def get_context_config(self):
return self.get_config()

def get_data_dir(self) -> str:
return str(self._ensure_plugin_data_dir())

def __getattr__(self, name: str) -> NullStub:
del name
return NullStub()
Expand Down
66 changes: 66 additions & 0 deletions tests/test_validate_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,72 @@ def test_load_plugins_index_rejects_non_dict_values(self):
os.remove(index_path)


class DummyContextStubTests(unittest.IsolatedAsyncioTestCase):
def test_dummy_context_defers_plugin_data_dir_creation_until_requested(self):
module = load_validator_module()

with tempfile.TemporaryDirectory() as tmp_dir:
astrbot_root = Path(tmp_dir) / "astrbot-root"
plugin_data_dir = astrbot_root / "data" / "plugin_data"

with mock.patch.dict(os.environ, {"ASTRBOT_ROOT": str(astrbot_root)}, clear=True):
context = module.DummyContext()
dir_exists_before = plugin_data_dir.exists()
created_dir = Path(context.get_data_dir())
dir_exists_after = plugin_data_dir.is_dir()

self.assertFalse(dir_exists_before)
self.assertEqual(created_dir.resolve(), plugin_data_dir.resolve())
self.assertTrue(dir_exists_after)

def test_dummy_context_returns_worker_data_dir_for_plugin_storage(self):
module = load_validator_module()

with tempfile.TemporaryDirectory() as tmp_dir:
astrbot_root = Path(tmp_dir) / "astrbot-root"
with mock.patch.dict(os.environ, {"ASTRBOT_ROOT": str(astrbot_root)}, clear=True):
data_dir = Path(module.DummyContext().get_data_dir())
data_dir_exists = data_dir.is_dir()

self.assertEqual(data_dir.resolve(), (astrbot_root / "data" / "plugin_data").resolve())
self.assertTrue(data_dir_exists)

async def test_null_stub_supports_async_database_context_pattern(self):
module = load_validator_module()

db = module.DummyContext().get_db()

async with db.get_db() as session:
self.assertIsInstance(session, module.NullStub)
async with session.begin() as transaction:
self.assertIs(transaction, session)
result = await session.execute("SELECT 1")

self.assertIs(result, session)

async def test_null_stub_returns_defaults_for_restart_style_config_access(self):
module = load_validator_module()

with mock.patch.dict(os.environ, {}, clear=True):
dashboard_config = module.DummyContext().get_config().get("dashboard", {})

self.assertEqual(dashboard_config.get("host", "127.0.0.1"), "127.0.0.1")
self.assertEqual(
int(os.environ.get("DASHBOARD_PORT", dashboard_config.get("port", 6185))),
6185,
)

def test_dummy_context_exposes_dict_like_config_defaults(self):
module = load_validator_module()

with mock.patch.dict(os.environ, {}, clear=True):
context = module.DummyContext()

self.assertEqual(context.get_config()["wake_prefix"], [])
self.assertEqual(context.get_config()["dashboard"].get("port", 6185), 6185)
self.assertEqual(context._config.get("expire_seconds", 300), 300)


class ValidationProgressTests(unittest.TestCase):
def test_build_parser_defaults_max_workers_to_sixteen(self):
module = load_validator_module()
Expand Down
Loading