fix: prefer named weekday cron examples#6091
fix: prefer named weekday cron examples#6091stablegenius49 wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: AstrBotTeam's Space pr4697的改动View Suggested Changes@@ -332,6 +332,8 @@
#### 相关工具
- `create_future_task`(原 create_cron_job):创建定时任务
+ - `cron_expression` 参数:使用标准 cron 表达式定义定时任务的执行周期,例如 `'0 8 * * *'`(每天早上8点)或 `'0 23 * * mon-fri'`(工作日晚上11点)
+ - **Cron 表达式建议**:推荐使用命名星期格式(如 `mon-fri` 或 `sat,sun`)而非数字范围(如 `1-5`),以避免不同 cron 实现之间的歧义。例如,`'0 23 * * mon-fri'` 比 `'0 23 * * 1-5'` 更清晰明确
- `delete_future_task`:删除定时任务
- `list_future_tasks`:列出所有定时任务
Note: You must be authenticated to accept/decline updates. |
There was a problem hiding this comment.
Hey - 我发现了两个问题,并给出了一些高层面的反馈:
- 新增的测试与描述文案的具体措辞强耦合(例如断言像 '1-5' 和 'avoid ambiguity' 这样的字面子串);建议放宽这些断言,把重点放在“包含对具名工作日的指引”上,而不是特定的措辞,这样将来对文案做微调时,就不会无谓地导致测试失败。
- 测试名
test_create_future_task_cron_description_prefers_named_weekdays仍然引用了create_future_task,而当前被测工具是CreateActiveCronTool;将测试重命名为与实际工具名一致,会让意图更清晰。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new test tightly couples to the exact wording of the description (e.g., asserting literal substrings like '1-5' and 'avoid ambiguity'); consider relaxing these expectations to focus on the presence of named weekday guidance rather than specific phrasing so future wording tweaks don't break the test needlessly.
- The test name `test_create_future_task_cron_description_prefers_named_weekdays` still refers to `create_future_task`, while the tool under test is `CreateActiveCronTool`; renaming the test to match the actual tool name would make intent clearer.
## Individual Comments
### Comment 1
<location path="astrbot/core/tools/cron_tools.py" line_range="33" />
<code_context>
"cron_expression": {
"type": "string",
- "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *').",
+ "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *' or '0 23 * * mon-fri'). Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity across cron implementations.",
},
"run_at": {
</code_context>
<issue_to_address>
**suggestion:** Clarify which cron dialect is expected rather than relying on generic implementation differences.
The note about named weekdays helps, but users still won’t know which cron semantics we follow (POSIX, Quartz, or a specific library). Please explicitly state the cron flavor or parser we use (e.g., "POSIX-style 5-field cron" or the library name) so users can rely on the correct external docs and avoid ambiguities like `0` vs `7` for Sunday or differing range behavior.
Suggested implementation:
```python
"description": "Cron expression defining recurring schedule using standard 5-field POSIX-style cron syntax (minute hour day-of-month month day-of-week), as parsed by this service's cron scheduler. For example: '0 8 * * *' or '0 23 * * mon-fri'. Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity around Sunday handling (0 vs 7) and differing cron implementations.",
```
If your project uses a specific cron library (e.g., `croniter`, `APScheduler`, `celery`, or a bespoke parser), you should further refine the description to name it explicitly. For example, replace “this service's cron scheduler” with “the `croniter` parser” or “APScheduler’s cron trigger semantics” so users can reference the appropriate external documentation.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_cron_tools.py" line_range="6-15" />
<code_context>
+from astrbot.core.tools.cron_tools import CreateActiveCronTool
+
+
+def test_create_future_task_cron_description_prefers_named_weekdays():
+ """The cron tool should steer users toward unambiguous named weekdays."""
+ tool = CreateActiveCronTool()
+
+ description = tool.parameters["properties"]["cron_expression"]["description"]
+
+ assert "mon-fri" in description
+ assert "sat,sun" in description
+ assert "1-5" in description
+ assert "avoid ambiguity" in description
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting that the original generic cron example is preserved as described in the PR summary.
The PR description specifies that the original generic example (e.g. `0 8 * * *`) should remain in addition to the new named weekday guidance, but this test only checks the new guidance. Adding an assertion like `assert "0 8 * * *" in description` would ensure both parts of the requirement are covered and better protect against regressions.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- The new test tightly couples to the exact wording of the description (e.g., asserting literal substrings like '1-5' and 'avoid ambiguity'); consider relaxing these expectations to focus on the presence of named weekday guidance rather than specific phrasing so future wording tweaks don't break the test needlessly.
- The test name
test_create_future_task_cron_description_prefers_named_weekdaysstill refers tocreate_future_task, while the tool under test isCreateActiveCronTool; renaming the test to match the actual tool name would make intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new test tightly couples to the exact wording of the description (e.g., asserting literal substrings like '1-5' and 'avoid ambiguity'); consider relaxing these expectations to focus on the presence of named weekday guidance rather than specific phrasing so future wording tweaks don't break the test needlessly.
- The test name `test_create_future_task_cron_description_prefers_named_weekdays` still refers to `create_future_task`, while the tool under test is `CreateActiveCronTool`; renaming the test to match the actual tool name would make intent clearer.
## Individual Comments
### Comment 1
<location path="astrbot/core/tools/cron_tools.py" line_range="33" />
<code_context>
"cron_expression": {
"type": "string",
- "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *').",
+ "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *' or '0 23 * * mon-fri'). Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity across cron implementations.",
},
"run_at": {
</code_context>
<issue_to_address>
**suggestion:** Clarify which cron dialect is expected rather than relying on generic implementation differences.
The note about named weekdays helps, but users still won’t know which cron semantics we follow (POSIX, Quartz, or a specific library). Please explicitly state the cron flavor or parser we use (e.g., "POSIX-style 5-field cron" or the library name) so users can rely on the correct external docs and avoid ambiguities like `0` vs `7` for Sunday or differing range behavior.
Suggested implementation:
```python
"description": "Cron expression defining recurring schedule using standard 5-field POSIX-style cron syntax (minute hour day-of-month month day-of-week), as parsed by this service's cron scheduler. For example: '0 8 * * *' or '0 23 * * mon-fri'. Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity around Sunday handling (0 vs 7) and differing cron implementations.",
```
If your project uses a specific cron library (e.g., `croniter`, `APScheduler`, `celery`, or a bespoke parser), you should further refine the description to name it explicitly. For example, replace “this service's cron scheduler” with “the `croniter` parser” or “APScheduler’s cron trigger semantics” so users can reference the appropriate external documentation.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_cron_tools.py" line_range="6-15" />
<code_context>
+from astrbot.core.tools.cron_tools import CreateActiveCronTool
+
+
+def test_create_future_task_cron_description_prefers_named_weekdays():
+ """The cron tool should steer users toward unambiguous named weekdays."""
+ tool = CreateActiveCronTool()
+
+ description = tool.parameters["properties"]["cron_expression"]["description"]
+
+ assert "mon-fri" in description
+ assert "sat,sun" in description
+ assert "1-5" in description
+ assert "avoid ambiguity" in description
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting that the original generic cron example is preserved as described in the PR summary.
The PR description specifies that the original generic example (e.g. `0 8 * * *`) should remain in addition to the new named weekday guidance, but this test only checks the new guidance. Adding an assertion like `assert "0 8 * * *" in description` would ensure both parts of the requirement are covered and better protect against regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "cron_expression": { | ||
| "type": "string", | ||
| "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *').", | ||
| "description": "Cron expression defining recurring schedule (e.g., '0 8 * * *' or '0 23 * * mon-fri'). Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity across cron implementations.", |
There was a problem hiding this comment.
suggestion: 澄清这里期望使用的是哪种 cron 方言,而不是笼统地依赖“不同实现行为不同”的说法。
关于使用具名工作日的说明是有帮助的,但用户仍然不知道我们实际遵循的是哪种 cron 语义(POSIX、Quartz,还是某个特定库)。请明确写出我们使用的 cron 风格或解析器(例如 "POSIX-style 5-field cron" 或具体库名),这样用户就可以参考正确的外部文档,并避免诸如星期天是 0 还是 7、以及区间行为差异之类的歧义。
建议的实现方式:
"description": "Cron expression defining recurring schedule using standard 5-field POSIX-style cron syntax (minute hour day-of-month month day-of-week), as parsed by this service's cron scheduler. For example: '0 8 * * *' or '0 23 * * mon-fri'. Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity around Sunday handling (0 vs 7) and differing cron implementations.",如果你的项目使用了某个特定的 cron 库(例如 croniter、APScheduler、celery,或自研解析器),应进一步在描述中将其名字写明。比如将 “this service's cron scheduler” 替换为 “the croniter parser” 或 “APScheduler’s cron trigger semantics”,这样用户就可以参考相应的外部文档。
Original comment in English
suggestion: Clarify which cron dialect is expected rather than relying on generic implementation differences.
The note about named weekdays helps, but users still won’t know which cron semantics we follow (POSIX, Quartz, or a specific library). Please explicitly state the cron flavor or parser we use (e.g., "POSIX-style 5-field cron" or the library name) so users can rely on the correct external docs and avoid ambiguities like 0 vs 7 for Sunday or differing range behavior.
Suggested implementation:
"description": "Cron expression defining recurring schedule using standard 5-field POSIX-style cron syntax (minute hour day-of-month month day-of-week), as parsed by this service's cron scheduler. For example: '0 8 * * *' or '0 23 * * mon-fri'. Prefer named weekdays like 'mon-fri' or 'sat,sun' instead of numeric day-of-week ranges such as '1-5' to avoid ambiguity around Sunday handling (0 vs 7) and differing cron implementations.",If your project uses a specific cron library (e.g., croniter, APScheduler, celery, or a bespoke parser), you should further refine the description to name it explicitly. For example, replace “this service's cron scheduler” with “the croniter parser” or “APScheduler’s cron trigger semantics” so users can reference the appropriate external documentation.
| def test_create_future_task_cron_description_prefers_named_weekdays(): | ||
| """The cron tool should steer users toward unambiguous named weekdays.""" | ||
| tool = CreateActiveCronTool() | ||
|
|
||
| description = tool.parameters["properties"]["cron_expression"]["description"] | ||
|
|
||
| assert "mon-fri" in description | ||
| assert "sat,sun" in description | ||
| assert "1-5" in description | ||
| assert "avoid ambiguity" in description |
There was a problem hiding this comment.
suggestion (testing): 建议同时断言原有的通用 cron 示例仍然保留,就像 PR 描述中所说的那样。
PR 描述中说明,原来的通用示例(例如 0 8 * * *)应该在新增具名工作日指引的基础上继续保留,但当前测试只检查了新增的指引。加上一条诸如 assert "0 8 * * *" in description 的断言,可以确保这两个部分的需求都被覆盖,从而更好地防止回归。
Original comment in English
suggestion (testing): Consider also asserting that the original generic cron example is preserved as described in the PR summary.
The PR description specifies that the original generic example (e.g. 0 8 * * *) should remain in addition to the new named weekday guidance, but this test only checks the new guidance. Adding an assertion like assert "0 8 * * *" in description would ensure both parts of the requirement are covered and better protect against regressions.
Summary
create_future_taskcron parameter description to recommend named weekdays likemon-fri/sat,sunTesting
PYTHONPATH=. pytest tests/unit/test_cron_tools.py tests/unit/test_cron_manager.py --test-profile=blockingCloses #5680.
Summary by Sourcery
在 cron 工具的元数据中澄清 cron 调度说明,并添加测试覆盖来强制执行新的描述。
增强内容:
测试:
Original summary in English
Summary by Sourcery
Clarify cron schedule guidance in the cron tool metadata and add test coverage to enforce the new description.
Enhancements:
Tests: