修复一个空指令bug#7
Open
AX-17 wants to merge 2 commits intoUnrealMultiple:masterfrom
Open
Conversation
There was a problem hiding this comment.
Hey - 我发现了 1 个问题,并给出了一些整体反馈:
- 在
RegisterEmptyCommands中,你先对空命令名称进行了规范化处理,然后又调用RemoveRegisteredEmptyCommands,而它内部会再次对名称进行规范化;建议要么对RemoveRegisteredEmptyCommands进行重载以接受已规范化的名称,要么重构逻辑,使规范化只发生一次,以避免额外的分配和枚举。 - 在
NormalizeEmptyCommandNames中,return [];语法依赖较新的 C# 特性;如果项目没有明确定位到该 C# 版本,使用Array.Empty<string>()来创建数组会更安全也更清晰。
给 AI Agent 的提示
请针对本次代码评审中的评论进行修改:
## 总体意见
- 在 `RegisterEmptyCommands` 中,你先对空命令名称进行了规范化处理,然后又调用 `RemoveRegisteredEmptyCommands`,而它内部会再次对名称进行规范化;建议要么对 `RemoveRegisteredEmptyCommands` 进行重载以接受已规范化的名称,要么重构逻辑,使规范化只发生一次,以避免额外的分配和枚举。
- 在 `NormalizeEmptyCommandNames` 中,`return [];` 语法依赖较新的 C# 特性;如果项目没有明确定位到该 C# 版本,使用 `Array.Empty<string>()` 来创建数组会更安全也更清晰。
## 逐条评论
### 评论 1
<location path="Vortex.Adapter/Plugin.cs" line_range="132" />
<code_context>
+ }
+ }
+
+ private static void RemoveRegisteredEmptyCommands(IEnumerable<string>? emptyCommands)
+ {
+ var normalizedNames = NormalizeEmptyCommandNames(emptyCommands);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 收紧删除谓词,以避免误删不相关的命令。
当前这个谓词同样可能匹配到真实命令,只要它们刚好只有一个名称、空权限,并且名称出现在 `nameSet` 中。如果这些删除操作只应该作用于该插件的占位命令,建议主要依赖 HelpText 标记(以及/或者来源程序集)来判定,并避免使用基于权限的回退逻辑,或者将那部分逻辑明确标记为临时/迁移用途。
建议实现:
```csharp
private static void RemoveRegisteredEmptyCommands(IEnumerable<string>? emptyCommands)
{
var normalizedNames = NormalizeEmptyCommandNames(emptyCommands);
var nameSet = new HashSet<string>(normalizedNames, StringComparer.OrdinalIgnoreCase);
// Only remove commands that:
// - Were created as this plugin's empty/placeholder commands (identified by HelpText marker), and
// - Have a name in the current empty-command name set.
//
// This avoids accidentally removing real commands that happen to have:
// - A single name,
// - An empty permission set, and
// - A name that overlaps with our empty-command list.
Commands.ChatCommands.RemoveAll(c =>
c.HelpText == EmptyCommandHelpText &&
nameSet.Contains(c.Name));
// NOTE: If you still need a temporary/migration path that also cleans up older
// placeholder commands that did not use the HelpText marker, implement that
// explicitly and time-bound here, e.g.:
//
// var migrationCutoff = new DateTime(2025, 01, 01, 0, 0, 0, DateTimeKind.Utc);
// Commands.ChatCommands.RemoveAll(c =>
// c.HelpText is null &&
// c.Permissions.Count == 0 &&
// nameSet.Contains(c.Name) &&
// c.OriginAssembly == typeof(Plugin).Assembly &&
// c.RegisteredAt < migrationCutoff);
//
// and remove that block once deployments are past the migration window.
}
```
我目前只能看到 `Plugin.cs` 的一小部分,请你仔细核对并调整 `SEARCH` 代码块,使其与现有的 `RemoveRegisteredEmptyCommands` 实现相匹配。你需要确保的关键行为变化是:
1. 对于正常的删除操作,`RemoveAll` 的谓词应当 *始终* 同时满足 `c.HelpText == EmptyCommandHelpText` 和 `nameSet.Contains(c.Name)`。
2. 任何额外条件(例如基于权限的)都应明确标注为临时/迁移用途的代码块,最好还要受 `OriginAssembly`(如果你的 `Command` 类型上提供该属性)和/或时间窗口的约束,并在不再需要时删除。
3. 如果你的 `Command` 类使用的程序集 / 插件来源属性名称不是 `OriginAssembly`,而是其他名字,请在你采用迁移示例时相应地更新这个引用。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- In
RegisterEmptyCommandsyou normalize the empty command names and then callRemoveRegisteredEmptyCommands, which normalizes them again; consider either overloadingRemoveRegisteredEmptyCommandsto accept already-normalized names or refactoring so normalization happens only once to avoid extra allocations and enumeration. - In
NormalizeEmptyCommandNames, thereturn [];syntax relies on newer C# features; if the project isn’t explicitly targeting that version, usingArray.Empty<string>()would be safer and clearer for array creation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RegisterEmptyCommands` you normalize the empty command names and then call `RemoveRegisteredEmptyCommands`, which normalizes them again; consider either overloading `RemoveRegisteredEmptyCommands` to accept already-normalized names or refactoring so normalization happens only once to avoid extra allocations and enumeration.
- In `NormalizeEmptyCommandNames`, the `return [];` syntax relies on newer C# features; if the project isn’t explicitly targeting that version, using `Array.Empty<string>()` would be safer and clearer for array creation.
## Individual Comments
### Comment 1
<location path="Vortex.Adapter/Plugin.cs" line_range="132" />
<code_context>
+ }
+ }
+
+ private static void RemoveRegisteredEmptyCommands(IEnumerable<string>? emptyCommands)
+ {
+ var normalizedNames = NormalizeEmptyCommandNames(emptyCommands);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten the removal predicate to avoid accidentally removing unrelated commands.
This predicate can also match real commands that happen to have a single name, an empty permission, and a matching name in `nameSet`. If these removals are meant to be limited to this plugin’s placeholder commands, consider keying primarily off the help-text marker (and/or origin assembly) and avoiding the permissions-based fallback, or making that path explicitly temporary/migration-only.
Suggested implementation:
```csharp
private static void RemoveRegisteredEmptyCommands(IEnumerable<string>? emptyCommands)
{
var normalizedNames = NormalizeEmptyCommandNames(emptyCommands);
var nameSet = new HashSet<string>(normalizedNames, StringComparer.OrdinalIgnoreCase);
// Only remove commands that:
// - Were created as this plugin's empty/placeholder commands (identified by HelpText marker), and
// - Have a name in the current empty-command name set.
//
// This avoids accidentally removing real commands that happen to have:
// - A single name,
// - An empty permission set, and
// - A name that overlaps with our empty-command list.
Commands.ChatCommands.RemoveAll(c =>
c.HelpText == EmptyCommandHelpText &&
nameSet.Contains(c.Name));
// NOTE: If you still need a temporary/migration path that also cleans up older
// placeholder commands that did not use the HelpText marker, implement that
// explicitly and time-bound here, e.g.:
//
// var migrationCutoff = new DateTime(2025, 01, 01, 0, 0, 0, DateTimeKind.Utc);
// Commands.ChatCommands.RemoveAll(c =>
// c.HelpText is null &&
// c.Permissions.Count == 0 &&
// nameSet.Contains(c.Name) &&
// c.OriginAssembly == typeof(Plugin).Assembly &&
// c.RegisteredAt < migrationCutoff);
//
// and remove that block once deployments are past the migration window.
}
```
I only see a small portion of `Plugin.cs`, so please double-check and adjust the `SEARCH` block to match your existing implementation of `RemoveRegisteredEmptyCommands`. The key behavior changes you want are:
1. The `RemoveAll` predicate should *always* require `c.HelpText == EmptyCommandHelpText` and `nameSet.Contains(c.Name)` for normal removals.
2. Any additional criteria (e.g., based on permissions) should be clearly marked as a temporary/migration-only block, ideally also constrained by `OriginAssembly` (if available on your `Command` type) and/or a time window, and removed once no longer needed.
3. If your `Command` class exposes an assembly / plugin origin property with a different name than `OriginAssembly`, update that reference accordingly in the migration example if you choose to implement it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
由 Sourcery 提供的总结
更新 Vortex 适配器插件的元数据,并改进对可配置的空占位符聊天指令的处理,包括正确的注册、清理和重载行为。
新功能:
错误修复:
增强改进:
Original summary in English
Summary by Sourcery
Update the Vortex adapter plugin metadata and improve handling of configurable empty placeholder chat commands, including proper registration, cleanup, and reload behavior.
New Features:
Bug Fixes:
Enhancements: