Skip to content

Conversation

Zhalslar
Copy link
Contributor

@Zhalslar Zhalslar commented Sep 4, 2025

Motivation

  • 当前components.py中每个组件的type属性都是直接字符串赋值,IDE会爆红。
  • 多个组件中当url为空时convert_to_base64检查路径导致的报错

Modifications

  • 每个组件的type属性改用使用本就定义好的ComponentType枚举类
  • 提前过滤多个组件中当url为空的情况

Check

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

Sourcery 总结

对所有组件类型属性使用 ComponentType 枚举,并为媒体转换方法添加早期验证

错误修复:

  • 在 Record 和 Image 组件中,当文件或 URL 字段缺失时,在转换为文件路径或 base64 之前引发异常

增强:

  • 将组件类型定义从原始字符串字面量转换为对基于字符串的 ComponentType 枚举的引用
  • 使用 or 表达式整合 URL/文件选择,并通过移除冗余的赋值步骤来简化路径解析
Original summary in English

Summary by Sourcery

Use the ComponentType enum for all component type attributes and add early validation to media conversion methods

Bug Fixes:

  • Raise exceptions when file or url fields are missing before converting to file paths or base64 in Record and Image components

Enhancements:

  • Convert component type definitions from raw string literals to references on the str-based ComponentType enum
  • Consolidate url/file selection using or expressions and simplify path resolution by removing redundant assignment steps

当前components.py中每个组件的type属性都是直接字符串赋值,IDE会爆红。
修正为使用本就定义好的ComponentType枚举类
用时修正多个组件中当url为空时convert_to_base64检查路径导致的报错
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.

大家好 - 我已审阅了您的更改,它们看起来很棒!

AI 代理提示
请处理此代码审查中的评论:
## 独立评论

### 评论 1
<location> `astrbot/core/message/components.py:471` </location>
<code_context>
-            return image_file_path
-        elif url and url.startswith("http"):
+        url = self.url or self.file
+        if not url:
+            raise ValueError("No valid file or URL provided")
+        if url.startswith("file:///"):
+            return url[8:]
</code_context>

<issue_to_address>
将缺少文件/URL 的情况切换为 ValueError 可以提高清晰度,但请检查类似方法之间的一致性。

请审查其他处理缺少文件或 URL 的方法,以确保它们也引发 ValueError,或者如果此模式很常见,则引入自定义异常。

建议的实现:

```python
    def get_audio_path(self):
        """
        获取音频的本地路径
        """
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
        if url.startswith("file:///"):
            return url[8:]
        elif url.startswith("http"):
            audio_file_path = await download_audio_by_url(url)
            return os.path.abspath(audio_file_path)
        # ... other logic ...

```

如果此文件或相关文件中还有其他类似方法(例如 `get_video_path``get_document_path` 等),您应该应用相同的模式:当既未提供文件也未提供 URL 时,引发 `ValueError`。如果此模式在许多地方使用,请考虑定义一个自定义异常(例如 `MissingFileOrUrlError`),并使用它而不是 `ValueError`,以获得更好的语义清晰度。
</issue_to_address>

### 评论 2
<location> `astrbot/core/message/components.py:470` </location>
<code_context>
-            image_file_path = url[8:]
-            return image_file_path
-        elif url and url.startswith("http"):
+        url = self.url or self.file
+        if not url:
+            raise ValueError("No valid file or URL provided")
</code_context>

<issue_to_address>
使用“or”进行 URL 选择可能会掩盖 self.url 为空字符串的情况。

如果 self.url 可能为空字符串,“or”将选择 self.file。请使用明确的检查来避免意外回退。
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
=======
        url = self.url
        if url is None or url == "":
            url = self.file
        if not url:
            raise ValueError("No valid file or URL provided")
>>>>>>> REPLACE

</suggested_fix>

Sourcery 对开源免费 - 如果您喜欢我们的评论,请考虑分享它们 ✨
帮助我更有用!请点击 👍 或 👎 对每条评论进行评价,我将利用这些反馈来改进我的评论。
Original comment in English

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `astrbot/core/message/components.py:471` </location>
<code_context>
-            return image_file_path
-        elif url and url.startswith("http"):
+        url = self.url or self.file
+        if not url:
+            raise ValueError("No valid file or URL provided")
+        if url.startswith("file:///"):
+            return url[8:]
</code_context>

<issue_to_address>
Switching to ValueError for missing file/URL improves clarity, but check for consistency across similar methods.

Please review other methods handling missing files or URLs to ensure they also raise ValueError, or introduce a custom exception if this pattern is frequent.

Suggested implementation:

```python
    def get_audio_path(self):
        """
        获取音频的本地路径
        """
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
        if url.startswith("file:///"):
            return url[8:]
        elif url.startswith("http"):
            audio_file_path = await download_audio_by_url(url)
            return os.path.abspath(audio_file_path)
        # ... other logic ...

```

If there are other similar methods (e.g., `get_video_path`, `get_document_path`, etc.) in this file or related files, you should apply the same pattern: raise `ValueError` when neither file nor URL is provided. If this pattern is used in many places, consider defining a custom exception (e.g., `MissingFileOrUrlError`) and use it instead of `ValueError` for better semantic clarity.
</issue_to_address>

### Comment 2
<location> `astrbot/core/message/components.py:470` </location>
<code_context>
-            image_file_path = url[8:]
-            return image_file_path
-        elif url and url.startswith("http"):
+        url = self.url or self.file
+        if not url:
+            raise ValueError("No valid file or URL provided")
</code_context>

<issue_to_address>
Using 'or' for url selection may mask cases where self.url is an empty string.

If self.url might be an empty string, 'or' will select self.file instead. Use an explicit check to avoid unintended fallback.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
=======
        url = self.url
        if url is None or url == "":
            url = self.file
        if not url:
            raise ValueError("No valid file or URL provided")
>>>>>>> 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.

Comment on lines +471 to +472
if not url:
raise ValueError("No valid file or URL provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: 将缺少文件/URL 的情况切换为 ValueError 可以提高清晰度,但请检查类似方法之间的一致性。

请审查其他处理缺少文件或 URL 的方法,以确保它们也引发 ValueError,或者如果此模式很常见,则引入自定义异常。

建议的实现:

    def get_audio_path(self):
        """
        获取音频的本地路径
        """
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
        if url.startswith("file:///"):
            return url[8:]
        elif url.startswith("http"):
            audio_file_path = await download_audio_by_url(url)
            return os.path.abspath(audio_file_path)
        # ... other logic ...

如果此文件或相关文件中还有其他类似方法(例如 get_video_pathget_document_path 等),您应该应用相同的模式:当既未提供文件也未提供 URL 时,引发 ValueError。如果此模式在许多地方使用,请考虑定义一个自定义异常(例如 MissingFileOrUrlError),并使用它而不是 ValueError,以获得更好的语义清晰度。

Original comment in English

suggestion: Switching to ValueError for missing file/URL improves clarity, but check for consistency across similar methods.

Please review other methods handling missing files or URLs to ensure they also raise ValueError, or introduce a custom exception if this pattern is frequent.

Suggested implementation:

    def get_audio_path(self):
        """
        获取音频的本地路径
        """
        url = self.url or self.file
        if not url:
            raise ValueError("No valid file or URL provided")
        if url.startswith("file:///"):
            return url[8:]
        elif url.startswith("http"):
            audio_file_path = await download_audio_by_url(url)
            return os.path.abspath(audio_file_path)
        # ... other logic ...

If there are other similar methods (e.g., get_video_path, get_document_path, etc.) in this file or related files, you should apply the same pattern: raise ValueError when neither file nor URL is provided. If this pattern is used in many places, consider defining a custom exception (e.g., MissingFileOrUrlError) and use it instead of ValueError for better semantic clarity.

Comment on lines +470 to +472
url = self.url or self.file
if not url:
raise ValueError("No valid file or URL provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: 使用“or”进行 URL 选择可能会掩盖 self.url 为空字符串的情况。

如果 self.url 可能为空字符串,“or”将选择 self.file。请使用明确的检查来避免意外回退。

Suggested change
url = self.url or self.file
if not url:
raise ValueError("No valid file or URL provided")
url = self.url
if url is None or url == "":
url = self.file
if not url:
raise ValueError("No valid file or URL provided")
Original comment in English

suggestion: Using 'or' for url selection may mask cases where self.url is an empty string.

If self.url might be an empty string, 'or' will select self.file instead. Use an explicit check to avoid unintended fallback.

Suggested change
url = self.url or self.file
if not url:
raise ValueError("No valid file or URL provided")
url = self.url
if url is None or url == "":
url = self.file
if not url:
raise ValueError("No valid file or URL provided")

return file_path
elif self.file and self.file.startswith("http"):
if not self.file:
raise Exception(f"not a valid file: {self.file}")
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): 抛出特定错误而不是通用的 ExceptionBaseException (raise-specific-error)

解释如果一段代码抛出特定异常类型 而不是通用的 [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) 或 [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), 则调用代码可以:
  • 获取有关错误类型的更多信息
  • 为其定义特定的异常处理

这样,代码的调用者就可以适当地处理错误。

您如何解决这个问题?

因此,与其让代码抛出 ExceptionBaseException,例如

if incorrect_input(value):
    raise Exception("The input is incorrect")

您可以让代码抛出特定错误,例如

if incorrect_input(value):
    raise ValueError("The input is incorrect")

或者

class IncorrectInputError(Exception):
    pass


if incorrect_input(value):
    raise IncorrectInputError("The input is incorrect")
Original comment in English

issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)

ExplanationIf a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:
  • get more information about what type of error it is
  • define specific exception handling for it

This way, callers of the code can handle the error appropriately.

How can you solve this?

So instead of having code raising Exception or BaseException like

if incorrect_input(value):
    raise Exception("The input is incorrect")

you can have code raising a specific error like

if incorrect_input(value):
    raise ValueError("The input is incorrect")

or

class IncorrectInputError(Exception):
    pass


if incorrect_input(value):
    raise IncorrectInputError("The input is incorrect")

@@ -197,12 +198,14 @@
str: 语音的 base64 编码,不以 base64:// 或者 data:image/jpeg;base64, 开头。
"""
# convert to base64
if self.file and self.file.startswith("file:///"):
if not self.file:
raise Exception(f"not a valid file: {self.file}")
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): 抛出特定错误而不是通用的 ExceptionBaseException (raise-specific-error)

解释如果一段代码抛出特定异常类型 而不是通用的 [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) 或 [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), 则调用代码可以:
  • 获取有关错误类型的更多信息
  • 为其定义特定的异常处理

这样,代码的调用者就可以适当地处理错误。

您如何解决这个问题?

因此,与其让代码抛出 ExceptionBaseException,例如

if incorrect_input(value):
    raise Exception("The input is incorrect")

您可以让代码抛出特定错误,例如

if incorrect_input(value):
    raise ValueError("The input is incorrect")

或者

class IncorrectInputError(Exception):
    pass


if incorrect_input(value):
    raise IncorrectInputError("The input is incorrect")
Original comment in English

issue (code-quality): Raise a specific error instead of the general Exception or BaseException (raise-specific-error)

ExplanationIf a piece of code raises a specific exception type rather than the generic [`BaseException`](https://docs.python.org/3/library/exceptions.html#BaseException) or [`Exception`](https://docs.python.org/3/library/exceptions.html#Exception), the calling code can:
  • get more information about what type of error it is
  • define specific exception handling for it

This way, callers of the code can handle the error appropriately.

How can you solve this?

So instead of having code raising Exception or BaseException like

if incorrect_input(value):
    raise Exception("The input is incorrect")

you can have code raising a specific error like

if incorrect_input(value):
    raise ValueError("The input is incorrect")

or

class IncorrectInputError(Exception):
    pass


if incorrect_input(value):
    raise IncorrectInputError("The input is incorrect")

@Zhalslar
Copy link
Contributor Author

Zhalslar commented Sep 4, 2025

AI的问题看起来没影响,实测正常运行,
下一步考虑将媒体组件中大量的重复代码提取出来复用,减少冗余,方便维护

@Soulter Soulter merged commit c1756e5 into AstrBotDevs:master Sep 6, 2025
2 of 3 checks passed
@Soulter Soulter changed the title fix: 修复组件type属性为枚举值 Fix: 修复组件type属性为枚举值 Sep 6, 2025
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.

2 participants