Skip to content

Conversation

anka-afk
Copy link
Member

@anka-afk anka-afk commented Sep 11, 2025

Motivation

不同插件之前需要方便地进行数据交互, 例如同步

Modifications

增加插件存取数据的封装, 并提供相应的监听器

Check

  • 😊 我的 Commit Message 符合良好的规范
  • 👀 我的更改经过良好的测试
  • 🤓 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到了 requirements.txtpyproject.toml 文件相应位置。
  • 😮 我的更改没有引入恶意代码

Sourcery 总结

添加共享数据存储和监听器 API,以促进插件间通信

新功能:

  • 引入 set_shared_dataget_shared_dataremove_shared_datalist_shared_data 方法,用于管理插件范围的共享数据
  • 实现 add_data_listenerremove_data_listener 方法,并提供异步通知以观察共享数据变化
Original summary in English

Summary by Sourcery

Add shared data storage and listener APIs to facilitate inter-plugin communication

New Features:

  • Introduce set_shared_data, get_shared_data, remove_shared_data, and list_shared_data methods for managing plugin-scoped shared data
  • Implement add_data_listener and remove_data_listener methods along with asynchronous notification to observe shared data changes

@auto-assign auto-assign bot requested review from Larch-C and Soulter September 11, 2025 10:27
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

你好 - 我已经审查了你的更改 - 这里有一些反馈:

  • 你有很多通过 inspect.currentframe 推断 plugin_name 的重复逻辑——将其提取到一个单独的辅助函数中,以减少重复和潜在的错误。
  • 考虑使用 asyncio.Lock(或类似机制)来保护对 _shared_data 和 _data_listeners 的访问,以避免多个任务并发调用 set/get 时出现竞态条件。
  • 依赖 inspect.currentframe 可能会创建引用循环并导致帧泄漏——考虑其他传递插件上下文的方法,或者至少确保帧被明确清除。
AI 代理提示
请处理此代码审查中的评论:
## 总体评论
- 你有很多通过 inspect.currentframe 推断 plugin_name 的重复逻辑——将其提取到一个单独的辅助函数中,以减少重复和潜在的错误。
- 考虑使用 asyncio.Lock(或类似机制)来保护对 _shared_data 和 _data_listeners 的访问,以避免多个任务并发调用 set/get 时出现竞态条件。
- 依赖 inspect.currentframe 可能会创建引用循环并导致帧泄漏——考虑其他传递插件上下文的方法,或者至少确保帧被明确清除。

## 单独评论

### 评论 1
<location> `astrbot/core/star/star_tools.py:557` </location>
<code_context>
+                    if asyncio.iscoroutine(task):
+                        tasks.append(task)
+                except Exception as e:
+                    logger.Error(f"数据监听器错误:{full_key}: {e}")
+
+            if tasks:
</code_context>

<issue_to_address>
日志方法名应小写以保持一致性。

将 'Error' 重命名为 'error' 以遵循日志约定并防止潜在的运行时问题。
</issue_to_address>

### 评论 2
<location> `astrbot/core/star/star_tools.py:533` </location>
<code_context>
+            plugin_name = metadata.name
+
+        full_key = f"{plugin_name}:{key}"
+        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
+            cls._data_listeners[full_key].remove(callback)
+            if not cls._data_listeners[full_key]:
</code_context>

<issue_to_address>
监听器移除不处理多个相同的回调。

目前,每次调用只移除一个回调实例。如果你想移除所有实例,请相应地更新逻辑或在文档中阐明此行为。
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
            cls._data_listeners[full_key].remove(callback)
            if not cls._data_listeners[full_key]:
                del cls._data_listeners[full_key]
            return True
        return False
=======
        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
            # Remove all instances of the callback from the listeners list
            cls._data_listeners[full_key] = [
                cb for cb in cls._data_listeners[full_key] if cb != callback
            ]
            if not cls._data_listeners[full_key]:
                del cls._data_listeners[full_key]
            return True
        return False
>>>>>>> REPLACE

</suggested_fix>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
帮助我更有用!请在每条评论上点击 👍 或 👎,我将使用这些反馈来改进你的评论。
Original comment in English

Hey there - I've reviewed your changes - here's some feedback:

  • You have a lot of repeated logic for inferring plugin_name via inspect.currentframe—extract that into a single helper to reduce duplication and potential mistakes.
  • Consider guarding access to _shared_data and _data_listeners with an asyncio.Lock (or similar) to avoid race conditions when multiple tasks call set/get concurrently.
  • Relying on inspect.currentframe can create reference cycles and leak frames—think about alternative ways to pass plugin context or at least ensure frames are explicitly cleared.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You have a lot of repeated logic for inferring plugin_name via inspect.currentframe—extract that into a single helper to reduce duplication and potential mistakes.
- Consider guarding access to _shared_data and _data_listeners with an asyncio.Lock (or similar) to avoid race conditions when multiple tasks call set/get concurrently.
- Relying on inspect.currentframe can create reference cycles and leak frames—think about alternative ways to pass plugin context or at least ensure frames are explicitly cleared.

## Individual Comments

### Comment 1
<location> `astrbot/core/star/star_tools.py:557` </location>
<code_context>
+                    if asyncio.iscoroutine(task):
+                        tasks.append(task)
+                except Exception as e:
+                    logger.Error(f"数据监听器错误:{full_key}: {e}")
+
+            if tasks:
</code_context>

<issue_to_address>
Logger method name should be lowercase for consistency.

Rename 'Error' to 'error' to follow logging conventions and prevent potential runtime issues.
</issue_to_address>

### Comment 2
<location> `astrbot/core/star/star_tools.py:533` </location>
<code_context>
+            plugin_name = metadata.name
+
+        full_key = f"{plugin_name}:{key}"
+        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
+            cls._data_listeners[full_key].remove(callback)
+            if not cls._data_listeners[full_key]:
</code_context>

<issue_to_address>
Listener removal does not handle multiple identical callbacks.

Currently, only one instance of the callback is removed per call. If you want to remove all instances, update the logic accordingly or clarify this behavior in the documentation.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
            cls._data_listeners[full_key].remove(callback)
            if not cls._data_listeners[full_key]:
                del cls._data_listeners[full_key]
            return True
        return False
=======
        if full_key in cls._data_listeners and callback in cls._data_listeners[full_key]:
            # Remove all instances of the callback from the listeners list
            cls._data_listeners[full_key] = [
                cb for cb in cls._data_listeners[full_key] if cb != callback
            ]
            if not cls._data_listeners[full_key]:
                del cls._data_listeners[full_key]
            return True
        return False
>>>>>>> REPLACE

</suggested_fix>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

1.重构抽取通用逻辑
2. 添加读写锁
3. 及时清除frame
Comment on lines 280 to 282
return data_dir.resolve()

return data_dir.resolve()
Copy link

Choose a reason for hiding this comment

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

这里写重复了

Copy link
Contributor

@LIghtJUNction LIghtJUNction left a comment

Choose a reason for hiding this comment

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

小建议

Copy link
Contributor

Choose a reason for hiding this comment

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

from typing import Union, Awaitable, List, Optional, ClassVar, Dict, Any, Callable
部分类型已过时,比如List,Dict,在3.9已经过时。应该改成内置的list,dict.Optional可以改成丨 None, Callable移动到了新库,以及类似的问题。
为了风格统一,便于维护,尽量完善下类型标注相关的工作。
except OSError as e:
if isinstance(e, PermissionError):
raise RuntimeError(f"无法创建目录 {data_dir}:权限不足") from e
raise RuntimeError(f"无法创建目录 {data_dir}:{e!s}") from e

没必要用isinstance判断错误类型。
这样写:
except PermissionError as e:
raise RuntimeError(f"无法创建目录 {data_dir}:权限不足") from e
except OSError as e:
raise RuntimeError(f"无法创建目录 {data_dir}:{e!s}") from e
更加美观

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants