Conversation
✅ Deploy Preview for nonebot-bison ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
suyiiyii
force-pushed
the
feat/all-target
branch
3 times, most recently
from
cb9d87e to
118d4cb
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces batch operations for cookie associations, allowing users to associate or disassociate cookies for multiple subscriptions at once through conversational interactions. The changes enhance the user experience by adding platform-specific filtering and batch processing capabilities.
- Add platform selection step before target selection for cookie operations
- Implement batch cookie association/disassociation for multiple targets
- Support "all" option to associate a cookie with all targets under a platform
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/sub_manager/test_add_cookie.py |
Updates test cases to reflect new multi-step workflow with platform selection |
nonebot_bison/sub_manager/utils.py |
Adds utility functions for platform selection and batch operations support |
nonebot_bison/sub_manager/del_cookie_target.py |
Refactors cookie deletion to support platform-based filtering and batch operations |
nonebot_bison/sub_manager/add_cookie_target.py |
Implements platform selection and batch cookie association functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
==========================================
+ Coverage 85.07% 85.19% +0.12%
==========================================
Files 99 99
Lines 5600 5742 +142
==========================================
+ Hits 4764 4892 +128
- Misses 836 850 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
suyiiyii
force-pushed
the
feat/all-target
branch
3 times, most recently
from
b03bbaa to
8c09215
Compare
suyiiyii
force-pushed
the
feat/all-target
branch
from
8c09215 to
c040bf2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| weibo_cookie = next(c for c in cookies if c.site_name == "weibo.com") | ||
| bilibili_cookie = next(c for c in cookies if c.site_name == "bilibili.com") |
There was a problem hiding this comment.
The test uses next(c for c in cookies if c.site_name == "weibo.com") which will raise StopIteration if no cookie matches. While this shouldn't happen in this test, consider using next(..., None) with a None check or a more explicit filter for better error handling and clarity in case of test setup issues.
| weibo_cookie = next(c for c in cookies if c.site_name == "weibo.com") | |
| bilibili_cookie = next(c for c in cookies if c.site_name == "bilibili.com") | |
| weibo_cookie = next((c for c in cookies if c.site_name == "weibo.com"), None) | |
| bilibili_cookie = next((c for c in cookies if c.site_name == "bilibili.com"), None) | |
| assert weibo_cookie is not None | |
| assert bilibili_cookie is not None |
| except Exception: | ||
| await del_cookie_target.reject("删除错误") | ||
| await del_cookie_target.reject("输入格式错误，请输入数字序号（多个用空格分隔）") |
There was a problem hiding this comment.
The outer except Exception block at line 68 catches any unexpected exception that wasn't handled by the inner try-except blocks. While the error message is generic, this is a reasonable safety net. However, this exception handler may never be reached in practice since all foreseeable exceptions (ValueError for invalid int parsing) are caught in the inner try-except. Consider whether this outer exception handler is necessary, or if it should log the unexpected exception for debugging purposes.
| from nonebug.app import App | ||
| import pytest |
There was a problem hiding this comment.
The imports are not organized according to PEP 8 style guidelines. Standard library imports should come before third-party imports. Move import pytest above from nonebug.app import App to follow the convention: standard library, blank line, third-party, blank line, local imports.
| from nonebug.app import App | |
| import pytest | |
| import pytest | |
| from nonebug.app import App |
| from nonebug.app import App | ||
| import pytest |
There was a problem hiding this comment.
The imports are not organized according to PEP 8 style guidelines. Standard library imports should come before third-party imports. Move import pytest above from nonebug.app import App to follow the convention: standard library, blank line, third-party, blank line, local imports.
| from nonebug.app import App | |
| import pytest | |
| import pytest | |
| from nonebug.app import App |
|
|
||
| res = "请输入想要关联 Cookie 的平台，目前支持，请输入冒号左边的名称：\n" | ||
| state["platform_table"] = {} | ||
| for platform_name in platforms: |
There was a problem hiding this comment.
The function iterates over platforms dict without sorting at line 166, while generate_cookie_target_platform_list_text uses sorted(platforms) at line 197. For consistency and better user experience, consider sorting the platforms in both functions to ensure a predictable and consistent display order.
| for platform_name in platforms: | |
| for platform_name in sorted(platforms): |
| except Exception: | ||
| await add_cookie_target_matcher.reject("序号错误") |
There was a problem hiding this comment.
The bare except Exception block catches all exceptions and provides a generic "序号错误" error message, which could hide other types of errors (e.g., KeyError when the index doesn't exist vs ValueError when converting to int). Consider catching specific exceptions separately to provide more helpful error messages. For instance, catch ValueError for invalid integer conversion and KeyError for missing index separately.
| except Exception: | |
| await add_cookie_target_matcher.reject("序号错误") | |
| except ValueError: | |
| # 用户输入的序号无法转换为整数 | |
| await add_cookie_target_matcher.reject("序号格式错误，请输入数字序号") | |
| except KeyError: | |
| # 用户输入的序号在订阅列表中不存在 | |
| await add_cookie_target_matcher.reject("序号不存在，请检查后重新输入") |
| except Exception: | ||
| # 这里可以根据需要获取target_name，但需要额外查询 | ||
| failed_targets.append(target_info["target"]) |
There was a problem hiding this comment.
The bare except Exception block at line 132 silently catches all exceptions during batch cookie target association. While failed targets are tracked, the specific error information is lost, making it difficult to diagnose why associations fail. Consider logging the exception or including error details in the failure report to aid debugging.
| except Exception: | ||
| target_info = f"{cookie_target.target.target_name}({cookie_target.cookie.cookie_name})" | ||
| failed_targets.append(target_info) |
There was a problem hiding this comment.
Similar to the batch association code, the bare except Exception block at line 81 silently catches all exceptions during batch cookie target deletion. While failed targets are tracked, the specific error information is lost. Consider logging the exception or including error details in the failure report to aid debugging.
| # Values should match keys | ||
| for key, value in platforms.items(): | ||
| assert key == value |
There was a problem hiding this comment.
The test assertion at line 119 verifies that dictionary keys equal their values (key == value), which is testing the exact behavior noted as potentially redundant in the implementation. If the dictionary structure is intentional for future extensibility, this test is fine. However, if the dict with identical key-value pairs is not needed, this test assertion should be updated accordingly when the implementation changes.
| return {ct.target.platform_name for ct in cookie_targets} | ||
|
|
||
|
|
||
| async def get_platform_cookie_targets(platform_name: str): |
There was a problem hiding this comment.
The function get_platform_cookie_targets is missing a return type annotation. Consider adding -> list or a more specific type annotation to maintain consistency with the rest of the codebase and improve code documentation.
| async def get_platform_cookie_targets(platform_name: str): | |
| async def get_platform_cookie_targets(platform_name: str) -> list: |
支持批量关联
支持批量取消关联
This change is