Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

添加Feishu通道 #1336

Open
wants to merge 4 commits into
base: browser-version
Choose a base branch
from

Conversation

IceThunder
Copy link
Contributor

No description provided.

添加Feishu通道的bot实例化
添加Feishu通道的依赖包
添加Feishu通道的bot基础功能(未完成)
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.

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

General suggestions:

  • Consider refactoring to reduce code duplication and improve maintainability.
  • Verify the asynchronous handling of functions to ensure they are correctly defined and used.
  • Review the implementation for potential concurrency issues given the use of global variables and threading.
Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Docstrings: all looks good

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 to tell me if it was helpful.

Comment on lines +81 to +92
class FeishuBot(BaseModel):
port: int = 9880
"""飞书回调端口号, 默认9880"""
debug: bool = False
"""是否开启debug,错误时展示日志"""
app_id: str
"""飞书应用 的 App ID"""
app_secret: str
"""飞书应用 的 Secret"""
token: str
"""飞书应用 API 加密策略 的 Verification Token"""
encrypt_key: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (code_refinement): Consider adding type hints for clarity and consistency.

Adding type hints for the class attributes would enhance code readability and maintain consistency with the rest of the codebase.

@@ -48,6 +48,11 @@
logger.info("检测到 Wecom 配置,将启动 Wecom Bot 模式……")
from platforms.wecom_bot import start_task

bots.append(loop.create_task(start_task()))
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Duplicate task creation for bots may lead to unintended behavior.

It appears that start_task() is being called and appended to bots twice in succession without any conditional checks in between. This might result in the same task being started twice unintentionally.

bot_request.set_result_status(RESPONSE_DONE)
bot_request.done = True
logger.debug(f"Bot request {bot_request.request_time} done.")
await reply(bot_request)
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): Verify the necessity of awaiting a non-async function.

The reply function does not appear to be an asynchronous function, yet it's being awaited. This might be an oversight. If reply is intended to be asynchronous, its definition needs to be updated accordingly.

@@ -78,6 +78,21 @@ class WecomBot(BaseModel):
"""企业微信应用 API 令牌 的 EncodingAESKey"""


class FeishuBot(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (complexity): Consider refactoring common bot configuration properties into a base class.

The addition of the FeishuBot class and its integration into the Config class introduces a higher level of complexity, primarily due to the increase in configuration parameters and the pattern of adding platform-specific bot configurations. While the implementation is consistent with the existing pattern, it's worth considering strategies to manage and potentially reduce complexity as the configuration grows.

One approach could be to refactor common properties among bot configurations into a base class. This base class could encapsulate properties like token, debug, and others that are shared across different bot configurations. Subsequent platform-specific configurations, like FeishuBot, could then extend this base class, adding or overriding properties as necessary. This would not only reduce redundancy but also simplify the addition of new bot configurations in the future.

Here's a simplified example of how this could be structured:

class BaseBotConfig(BaseModel):
    port: int
    debug: bool = False
    token: str
    app_id: Optional[str] = None
    app_secret: Optional[str] = None
    encrypt_key: Optional[str] = None

class FeishuBot(BaseBotConfig):
    port: int = 9880
    app_id: str
    app_secret: str
    # Inherits debug, token, and encrypt_key from BaseBotConfig

class Config(BaseModel):
    # Other platform settings...
    feishu: Optional[FeishuBot] = None
    # Account Settings...

This approach could make the codebase easier to maintain and extend, especially as more configurations are added. It's a suggestion to consider how the current implementation might evolve and how to manage complexity effectively.

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.

1 participant