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

feat: 支持CompositeCommand间组合 #2125

Closed
wants to merge 12 commits into from

Conversation

hundun000
Copy link
Contributor

@hundun000 hundun000 commented Jun 30, 2022

效果:

改动说明:

  • 新增注解@ChildCommad也可以考虑继续使用@SubCommad?以及child的命名可能会引起歧义想到继承关系
  • 需要将数个CommandReflector的方法改为返回接口类而不是实现类

Copy link
Member

@Him188 Him188 left a comment

Choose a reason for hiding this comment

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

请执行 ./gradlew :mirai-console:apiDumpAll 并提交变更的文件

@Him188 Him188 added t:feature 类型: 新特性 s:console 子系统: mirai-console labels Jun 30, 2022
@Him188 Him188 self-requested a review June 30, 2022 15:18
@hundun000
Copy link
Contributor Author

看了下英文下描述组合模式也会使用child,那就保持@ChildCommad命名。测试里的Parent改名Container减少歧义。

@hundun000 hundun000 marked this pull request as ready for review July 1, 2022 06:15
Copy link
Member

@Karlatemp Karlatemp left a comment

Choose a reason for hiding this comment

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

如果使用 ChildCommand 的话, 她给我的第一印象是

object TopLevel : CompositeCommand("toplevel") {
   val myCommand = object : CompositeCommand() {
       @SubCommand
       suspend fun CommandSender.handle() {}
   }
}

命令会按照 /toplevel myCommand handle 注册, 我个人感觉这个名字会有很大的误解性.
也许可以考虑 @CombinedCommand 或者 @MixinedCommand? @Him188

@Him188
Copy link
Member

Him188 commented Jul 1, 2022

@Karlatemp 我也是这样想的, 今天忙, 看了没来得及写 review

我认为造一个其他概念而不要用 CompositeCommand 比较好. 比如创造一个 SubCommandGroup, 考虑定义为接口, 也需要考虑与 CompositeCommand 的兼容性 (CompositeCommand 是否能直接使用 SubCommandGroup 的现有指令定义?)

@hundun000
Copy link
Contributor Author

另外现在这样,CompositeCommand间可以进一步嵌套“父节点-若干子节点”关系形成树状。不知道要不要设计上就禁止,区分出'SubCommandGroup'应该有利于实现这点。

@hundun000
Copy link
Contributor Author

不过单看“XX助手”场景,有的功能点可能由SimpleCommand提供,所以需要的是能混合组合SimpleCommand和CompositeCommand 的工具。从这个角度来看,SubCommandGroup应该支持组合Command,也就包括支持SubCommandGroup间嵌套成树状。

@Him188
Copy link
Member

Him188 commented Jul 1, 2022

我考虑了 hierarchy 的, SubCommandGroup 这个命名能体现层级, 你设计的 val 声明语法也适合做到这一点

SimpleCommand 实际上是只有一个子指令的 CompositeCommand. SubCommandGroup 仍然能做到, 这就涉及我说的考虑兼容性(复用的可能性)问题.

@hundun000
Copy link
Contributor Author

hundun000 commented Jul 2, 2022

SimpleCommand 实际上是只有一个子指令的 CompositeCommand

看了下代码, SimpleCommandCompositeCommand只是平级的关系啊。是想说SimpleCommandSubCommandAnnotationResolver是一种findSubCommands + validate的结果只有一个子指令的Resolver吗?

另外我还是不太理解你说的兼容性和复用性指什么,能更具体的说明吗?

@Him188
Copy link
Member

Him188 commented Jul 2, 2022

@hundun000 是的

兼容性指的是有没有可能让开发者用 SubCommandGroup 写, 然后 CompositeCommand 可以使用这个 group 而不需要重新定义

@hundun000
Copy link
Contributor Author

@Him188

我认为造一个其他概念而不要用 CompositeCommand 比较好

我的理解是要让 CompositeCommand 不变,使其和SubCommandGroup有如下区别

成员函数作为子指令 得到成员属性提供的子指令
SimpleCommand √(仅对@Handler x
CompositeCommand √(仅对@SubCommand x
SubCommandGroup 允许 or 不允许? √(仅对@CombinedCommand

开发者用 SubCommandGroup 写, 然后 CompositeCommand 可以使用这个 group

是指变成这样吗,那不就又没区别了?

成员函数作为子指令 得到成员属性提供的子指令
CompositeResolver √(仅对@SubCommand
SubCommandGroup √(仅对@CombinedCommand

@Him188
Copy link
Member

Him188 commented Jul 3, 2022

SubCommandGroup 是对 SubCommand 的集合, 没有主指令名称, 它不能独立使用. CompositeCommand 拥有主指令名称, 包含一个 SubCommandGroup 时就包含了它的所有 SubCommand. 这样拥有最高的实用性?

@hundun000
Copy link
Contributor Author

SubCommandGroup 是对 SubCommand 的集合, 没有主指令名称, 它不能独立使用. CompositeCommand 拥有主指令名称, 包含一个 SubCommandGroup 时就包含了它的所有 SubCommand. 这样拥有最高的实用性?

懂了

@hundun000
Copy link
Contributor Author

hundun000 commented Jul 4, 2022

  1. AbstractSubCommandGroup是接口SubCommandGroup的一种实现,实现方式是交给SubCommandReflector计算;AbstractSubCommandGroup不是Command,即不能独立使用。
  2. @SubCommand是protected于CompositeCommand的,故AbstractSubCommandGroup需要自己的注解,暂时使用@AnotherSubCommand。是否应该让@SubCommand变为public供两者统一使用?其他@AnotherXXX同理。
  3. 为了识别各种@AnotherXXX,需要新的Resolver——GroupedCommandSubCommandAnnotationResolver,且SubCommandAnnotationResolver需要变为带泛型参数。
  4. CommandReflector理解成两部分功能:一是反射出子指令,委托给SubCommandReflector;二是反射出子指令以外的Command属性(目前仅Usage)。

@hundun000 hundun000 requested a review from Karlatemp July 13, 2022 01:25
Copy link
Member

@Him188 Him188 left a comment

Choose a reason for hiding this comment

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

总体来说是很不错的实现, 有一些考虑:

  • SubCommandGroup 最好要提供一个可选的名称前缀的功能, 就像 CompositeCommand 那样. 我建议在使用注解时
  • 在实现上一点后, CompositeCommand 实际上也是一个 SubCommandGroup. 把 SubCommandGroupprovideOverloads 改为 overloads, CompositeCommandAbstractSubCommandGroup 就可以都继承 SubCommandGroup. 这解决了各种类型兼容问题, 但这将会允许 CompositeCommandSubCommandGroup 互相嵌套. 嵌套方案在之后讨论
  • 互相嵌套时 CommandReflector 会不会报错? 需要增加测试
  • 需要增加对 generateUsage 的测试

有关我上面说的设计方案, 由于 console 的指令是定义式的, 我想象了如下使用 group 的方式, 欢迎评论:

// 插件一个模块的部分功能
class ModuleAPart1 : AbstractSubCommandGroup() {
    @SubCommand
    fun function1();
}

// 插件一个模块的另一部分功能
class ModuleAPart2 : AbstractSubCommandGroup() {
    @SubCommand
    fun function2();
    @SubCommand
    fun function3();
}

// 插件的一个模块, 统合上面两个功能
class ModuleACommandGroup: AbstractSubCommandGroup() {
    @SubCommand // 与在函数上标注这个注解类似, 它会带 `part1` 这个名称前缀来注册指令. 需要执行  /base part1 function1, 也可以使用 SubCommand 的参数来覆盖名称
    val part1: ModuleAPart1
    @FlattenSubCommands // 新增, 不带前缀注册指令, 执行 /base function2
    val part1: ModuleAPart1
}

// 插件的总指令, 统合全部模块
class MyUnifiedCommand : CompositeCommand() {
    @SubCommand 
    val moduleA: ModuleACommandGroup

    @SubCommand // 与现有注册方式混合, 看起来也不错
    fun about();
}

注解方案

新设计的 CompositeCommandSubCommandGroup 在定义指令上是一样的, 我希望让它们使用同样的注解. 我觉得可以把那些 @SubCommand 复制到新抽象的 SubCommandGroup 中并 public. 将旧的标注 @Deprecated(level = HIDDEN). 这样 Kotlin 用户只需要修复 import (因为 Kotlin 引用非直接父类的内部类需要带外部类类名), Java 用户无需更新.

嵌套方案

嵌套是否需要增加前缀, 在且只在外层定义中指定.

  • 使用 @SubCommand 时, 增加前缀. 前缀的名称的行为与这个注解标注在指令上时一致——有参数使用参数, 无参数使用指令反射名称
  • 使用新增的 @FlattenSubCommands (这是我建议的名称, 可考虑更好名称) 时, 不增加前缀. 这样的行为类似标准库集合的 flatMap.

有关混合嵌套问题, 我考虑:

  • 允许 AbstractSubCommandGroup 嵌入 CompositeCommand——原 PR 计划的功能
  • 允许 CompositeCommand 嵌入 AbstractSubCommandGroup, 即使这可能导致 confusion, 但这毕竟是用户自己要这么干的. 以后可考虑通过 IDE 插件提供预览来解决这个问题 (指令增加 group 之后变得复杂, 也确实可能需要预览了)

有关用户自己实现接口的考虑:
接口公开, 且容易实现. 我认为可以允许用户实现, 只要符合接口的设计规范.

Comment on lines 141 to 173
public constructor(
ownerCommand: Command,
owner: Any,
correspondingFunction: KFunction<*>,
message: String?,
) : super("Illegal command declaration: ${correspondingFunction.name} declared in ${ownerCommand::class.qualifiedName}") {
) : super("Illegal command declaration: ${correspondingFunction.name} declared in ${owner::class.qualifiedName}") {
this.message = message
}
Copy link
Member

Choose a reason for hiding this comment

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

增加一个适用于 CommandGroup 的构造器, 而不是修改参数为 Any

@hundun000
Copy link
Contributor Author

hundun000 commented Aug 2, 2022

我觉得可以把那些 @SubCommand 复制到新抽象的 SubCommandGroup 中并 public. 将旧的标注 @Deprecated(level = HIDDEN). 这样 Kotlin 用户只需要修复 import (因为 Kotlin 引用非直接父类的内部类需要带外部类类名), Java 用户无需更新.

即使已经(level = HIDDEN)且CompositeCommand extends SubCommandGroup,IDE还是会认为用户在试图使用父类的@SubCommand 并报错。import …SubCommandGroup.SubCommand未被使用,用户需要显式写成@SubCommandGroup.SubCommand

计划开发期间真实移除CompositeCommand.SubCommand防止增加多处改动,晚些时候再讨论是否恢复为@Deprecated

image

类似问题的issue已被记录为kotlin bug。

@hundun000 hundun000 marked this pull request as draft August 2, 2022 10:40
@Him188
Copy link
Member

Him188 commented Aug 2, 2022

@hundun000

真实移除是 ABI 变更,是不可能进行的,只能考虑其他方案了...

@Him188
Copy link
Member

Him188 commented Aug 20, 2022

@hundun000 @Karlatemp 有什么想法吗,关于新注解

@hundun000
Copy link
Contributor Author

hundun000 commented Aug 22, 2022

x Kotlin 用户只需要修复 import
√ Kotlin 用户修复为显式使用@SubCommandGroup.SubCommand

这样?因为我看IDEA自动补全也是选择补全为这样(指以前CompositeCommand.SubCommand还未废弃的时候),是因为这样也更符合编码规范吗?

@Him188
Copy link
Member

Him188 commented Aug 22, 2022

这个就是 Kotlin 设计问题,不允许用 simple name 引用超过一级继承关系的内部类型。Java 是可以的

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:console 子系统: mirai-console t:feature 类型: 新特性
Projects
None yet
Development

Successfully merging this pull request may close these issues.

支持 多个主指令名相同的CompositeCommand注册的SubCommand均生效
3 participants