Dev6#1560
Merged
Merged
Conversation
- 移除鬼灵歌姬御魂的补充配置字段 - 移除极鬼灵歌姬御魂的补充配置字段 - 更新隐藏字段列表以反映配置变更 - 删除周一特定的御魂切换逻辑 - 添加未知御魂配置的日志记录
- 删除重复的 switch_soul_config_1 和 switch_soul_config_2 配置项 - 将两个配置合并为单个 switch_soul_config 配置项 - 更新任务脚本中的配置引用以使用新的单一配置 - 移除重复的御魂切换逻辑代码
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体反馈：
- 将
switch_soul_config_1/2合并为单一的switch_soul_config可能会破坏已有配置；可以考虑在配置模型中增加字段别名或迁移/兼容层，以确保旧配置仍然能够被正确反序列化。 - 在
checkout_soul中，你现在在第一次成功调用run_switch_soul_by_name后就直接return；如果调用方还依赖于切换御魂之后的后续逻辑，可能需要避免这种提前返回，或者把该方法的返回行为说明得更清楚。 - 移除
*_supplementary鬼王（demon）配置字段以及与周一相关的逻辑，会在无提示的情况下改变行为；可以考虑是否需要一个回退或废弃路径，这样出现意料之外的空group,team值时，不仅仅是写个错误日志，而是能更明确地反馈给用户或调用方。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- Removing `switch_soul_config_1/2` in favor of a single `switch_soul_config` may break existing configs; consider adding field aliases or a migration/compat layer in the config model so old configurations still deserialize correctly.
- In `checkout_soul`, you now `return` after the first successful `run_switch_soul_by_name`; if callers rely on subsequent logic after soul switching, you may want to avoid the early return or make the method’s return behavior explicit.
- The removal of the `*_supplementary` demon config fields and logic for Monday changes behavior silently; consider whether you need a fallback or deprecation path so unexpected empty `group,team` values don’t just log an error but are surfaced more explicitly to the user or caller.
## Individual Comments
### Comment 1
<location path="tasks/SixRealms/script_task.py" line_range="34-38" />
<code_context>
def run(self):
- if self._config.switch_soul_config_1.enable:
+ if self._config.switch_soul_config.enable:
self.ui_get_current_page()
self.ui_goto(page_shikigami_records)
- self.run_switch_soul(self._config.switch_soul_config_1.switch_group_team)
- if self._config.switch_soul_config_1.enable_switch_by_name:
+ self.run_switch_soul(self._config.switch_soul_config.switch_group_team)
+ if self._config.switch_soul_config.enable_switch_by_name:
self.ui_get_current_page()
self.ui_goto(page_shikigami_records)
</code_context>
<issue_to_address>
**suggestion:** 考虑在两个标志都启用时避免对 `page_shikigami_records` 的重复跳转。
合并为单一的 `switch_soul_config` 之后，当 `enable` 和 `enable_switch_by_name` 都为 true 时，这条路径会连续两次调用 `ui_get_current_page()` 和 `ui_goto(page_shikigami_records)`。你可以改为只导航一次，然后依次调用 `run_switch_soul` 和 `run_switch_soul_by_name`，以避免多余的 UI 调用并降低潜在的不稳定性。
</issue_to_address>
### Comment 2
<location path="tasks/DemonEncounter/script_task.py" line_range="68-69" />
<code_context>
- else:
- group, team = getattr(self.conf.demon_soul_config, f'{self.boss_type}_supplementary').split(",")
- self.run_switch_soul_by_name(group, team)
+ return
+ logger.error(f'Unknown switch soul conf: group[{group}], team[{team}]')
def execute_boss(self):
</code_context>
<issue_to_address>
**question (bug_risk):** 移除周一的补充御魂切换逻辑会改变行为；请确认这是有意为之。
此前，当 `datetime.now().weekday() == 0` 时，代码会进行第二次切换，切到 `*_supplementary` 御魂（对普通和最佳鬼王都是如此）。现在在第一次 `run_switch_soul_by_name` 后就直接返回，因此不会再执行补充切换。如果玩家依赖这种“周一双阶段切换”的行为，他们的体验会发生变化。如果这是有意的，则无需改动；否则建议在保留新配置结构的前提下，重新引入周一特有的补充切换逻辑。
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Removing
switch_soul_config_1/2in favor of a singleswitch_soul_configmay break existing configs; consider adding field aliases or a migration/compat layer in the config model so old configurations still deserialize correctly. - In
checkout_soul, you nowreturnafter the first successfulrun_switch_soul_by_name; if callers rely on subsequent logic after soul switching, you may want to avoid the early return or make the method’s return behavior explicit. - The removal of the
*_supplementarydemon config fields and logic for Monday changes behavior silently; consider whether you need a fallback or deprecation path so unexpected emptygroup,teamvalues don’t just log an error but are surfaced more explicitly to the user or caller.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing `switch_soul_config_1/2` in favor of a single `switch_soul_config` may break existing configs; consider adding field aliases or a migration/compat layer in the config model so old configurations still deserialize correctly.
- In `checkout_soul`, you now `return` after the first successful `run_switch_soul_by_name`; if callers rely on subsequent logic after soul switching, you may want to avoid the early return or make the method’s return behavior explicit.
- The removal of the `*_supplementary` demon config fields and logic for Monday changes behavior silently; consider whether you need a fallback or deprecation path so unexpected empty `group,team` values don’t just log an error but are surfaced more explicitly to the user or caller.
## Individual Comments
### Comment 1
<location path="tasks/SixRealms/script_task.py" line_range="34-38" />
<code_context>
def run(self):
- if self._config.switch_soul_config_1.enable:
+ if self._config.switch_soul_config.enable:
self.ui_get_current_page()
self.ui_goto(page_shikigami_records)
- self.run_switch_soul(self._config.switch_soul_config_1.switch_group_team)
- if self._config.switch_soul_config_1.enable_switch_by_name:
+ self.run_switch_soul(self._config.switch_soul_config.switch_group_team)
+ if self._config.switch_soul_config.enable_switch_by_name:
self.ui_get_current_page()
self.ui_goto(page_shikigami_records)
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding double navigation to `page_shikigami_records` when both flags are enabled.
With the merged `switch_soul_config`, when both `enable` and `enable_switch_by_name` are true, this path calls `ui_get_current_page()` and `ui_goto(page_shikigami_records)` twice in succession. You could instead navigate once and then run both `run_switch_soul` and `run_switch_soul_by_name` to avoid redundant UI calls and reduce potential flakiness.
</issue_to_address>
### Comment 2
<location path="tasks/DemonEncounter/script_task.py" line_range="68-69" />
<code_context>
- else:
- group, team = getattr(self.conf.demon_soul_config, f'{self.boss_type}_supplementary').split(",")
- self.run_switch_soul_by_name(group, team)
+ return
+ logger.error(f'Unknown switch soul conf: group[{group}], team[{team}]')
def execute_boss(self):
</code_context>
<issue_to_address>
**question (bug_risk):** Removal of Monday supplementary soul switching changes behavior; confirm this is intended.
Previously, when `datetime.now().weekday() == 0`, the code did a second switch to a `*_supplementary` soul (for both normal and best demons). Now it returns after the first `run_switch_soul_by_name`, so the supplementary switch never occurs. If players depended on this two-stage Monday behavior, their experience will change. If that’s intentional, no action needed; if not, consider reintroducing the Monday-specific supplementary switch while keeping the new config structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+34
to
+38
| if self._config.switch_soul_config.enable: | ||
| self.ui_get_current_page() | ||
| self.ui_goto(page_shikigami_records) | ||
| self.run_switch_soul(self._config.switch_soul_config_1.switch_group_team) | ||
| if self._config.switch_soul_config_1.enable_switch_by_name: | ||
| self.run_switch_soul(self._config.switch_soul_config.switch_group_team) | ||
| if self._config.switch_soul_config.enable_switch_by_name: |
There was a problem hiding this comment.
suggestion: 考虑在两个标志都启用时避免对 page_shikigami_records 的重复跳转。
合并为单一的 switch_soul_config 之后，当 enable 和 enable_switch_by_name 都为 true 时，这条路径会连续两次调用 ui_get_current_page() 和 ui_goto(page_shikigami_records)。你可以改为只导航一次，然后依次调用 run_switch_soul 和 run_switch_soul_by_name，以避免多余的 UI 调用并降低潜在的不稳定性。
Original comment in English
suggestion: Consider avoiding double navigation to page_shikigami_records when both flags are enabled.
With the merged switch_soul_config, when both enable and enable_switch_by_name are true, this path calls ui_get_current_page() and ui_goto(page_shikigami_records) twice in succession. You could instead navigate once and then run both run_switch_soul and run_switch_soul_by_name to avoid redundant UI calls and reduce potential flakiness.
Comment on lines
+68
to
+69
| return | ||
| logger.error(f'Unknown switch soul conf: group[{group}], team[{team}]') |
There was a problem hiding this comment.
question (bug_risk): 移除周一的补充御魂切换逻辑会改变行为；请确认这是有意为之。
此前，当 datetime.now().weekday() == 0 时，代码会进行第二次切换，切到 *_supplementary 御魂（对普通和最佳鬼王都是如此）。现在在第一次 run_switch_soul_by_name 后就直接返回，因此不会再执行补充切换。如果玩家依赖这种“周一双阶段切换”的行为，他们的体验会发生变化。如果这是有意的，则无需改动；否则建议在保留新配置结构的前提下，重新引入周一特有的补充切换逻辑。
Original comment in English
question (bug_risk): Removal of Monday supplementary soul switching changes behavior; confirm this is intended.
Previously, when datetime.now().weekday() == 0, the code did a second switch to a *_supplementary soul (for both normal and best demons). Now it returns after the first run_switch_soul_by_name, so the supplementary switch never occurs. If players depended on this two-stage Monday behavior, their experience will change. If that’s intentional, no action needed; if not, consider reintroducing the Monday-specific supplementary switch while keeping the new config structure.
Change Summary
Reconstructed Intent点击此处展开这次改动最可能是为了:
Observed Constraints点击此处展开
Intent Alignment
Release Risk
Validation Gaps点击此处展开
|
Findings
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
将六道轮回的灵魂切换配置统一为一个配置项，并简化针对桐生歌姬（Kiryou Utahime）Boss 的魔物遭遇（Demon Encounter）灵魂选择逻辑。
Bug 修复：
增强改进：
switch_soul_config。Original summary in English
Summary by Sourcery
Unify Six Realms soul-switch configuration to a single config entry and simplify Demon Encounter soul selection for Kiryou Utahime bosses.
Bug Fixes:
Enhancements: