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.

Sign up for GitHub

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

Jump to bottom

fix #764 #765

Closed
wants to merge 0 commits into from
Closed

fix #764 #765

wants to merge 0 commits into from

Conversation

BH4HPA
Copy link

@BH4HPA BH4HPA commented Jul 27, 2022 •
edited
Loading

fix #764

shigma
shigma requested changes Jul 27, 2022
View reviewed changes
Copy link
Member

@shigma shigma left a comment

Choose a reason for hiding this comment

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

目前的 Command.Config 不用于核验，你需要同时修改 Command.defaultConfig。

@BH4HPA BH4HPA requested a review from shigma July 28, 2022 00:35
shigma
shigma requested changes Jul 28, 2022
View reviewed changes
Copy link
Member

@shigma shigma left a comment

Choose a reason for hiding this comment

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

Oops, you should update the unit tests as well. The changes introduced some failing tests:

  • packages/core/tests/command.spec.ts
  • packages/core/tests/runtime.spec.ts

@BH4HPA BH4HPA requested a review from shigma July 29, 2022 03:07
@BH4HPA
Copy link
Author

BH4HPA commented Jul 31, 2022

目前发现一个问题，启用 checkArgCount 之后，将无法使用 command -h 来获取帮助。
sandbox example

@BH4HPA
Copy link
Author

BH4HPA commented Jul 31, 2022 •
edited
Loading

重定向至获取指令帮助这里的代码应该没问题

koishi/packages/core/src/command/runtime.ts

Lines 70 to 74 in f8680d9

ctx.before('command/execute', (argv) => {
const { args, command, options, session } = argv
if (options['help'] && command._options.help) {
return executeHelp(session, command.name)
}

document

koishi/packages/core/src/command/validate.ts

Lines 47 to 64 in f8680d9

// check argv
ctx.before('command/execute', (argv: Argv) => {
const { args, options, command, session } = argv
function sendHint(message: string, ...param: any[]) {
return command.config.showWarning ? session.text(message, param) : ''
}
// check argument count
if (command.config.checkArgCount) {
const nextArg = command._arguments[args.length] || {}
if (nextArg.required) {
return sendHint('internal.insufficient-arguments')
}
const finalArg = command._arguments[command._arguments.length - 1] || {}
if (args.length > command._arguments.length && finalArg.type !== 'text' && !finalArg.variadic) {
return sendHint('internal.redunant-arguments')
}
}

checkArgCount 的实现也是通过注册 command/before-execute 事件来实现的，但注册的好像更晚

koishi/packages/core/src/command/index.ts

Lines 49 to 53 in f8680d9

constructor(private ctx: Context, private config: Commander.Config = {}) {
defineProperty(this, Context.current, ctx)
ctx.plugin(runtime)
ctx.plugin(validate)
}

@BH4HPA BH4HPA mentioned this pull request Jul 31, 2022
Closed
@codecov
Copy link

codecov bot commented Jul 31, 2022 •
edited
Loading

Codecov Report

Merging #765 (f80338d) into master (f80338d) will not change coverage.
The diff coverage is n/a.

❗ Current head f80338d differs from pull request most recent head 4393d47. Consider uploading reports for the commit 4393d47 to get more accurate results

@@           Coverage Diff           @@
##           master     #765   +/-   ##
=======================================
  Coverage   94.72%   94.72%           
=======================================
  Files          57       57           
  Lines        6142     6142           
  Branches     1229     1229           
=======================================
  Hits         5818     5818           
  Misses        324      324

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f80338d...4393d47. Read the comment docs.

@BH4HPA
Copy link
Author

BH4HPA commented Jul 31, 2022

after a0d32ee, with 68263b3, i can pass unit test at my local now.

144 passing (2s)

Done in 12.43s.

MaikoTan
MaikoTan reviewed Sep 2, 2022
View reviewed changes
Copy link
Member

@MaikoTan MaikoTan left a comment

Choose a reason for hiding this comment

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

Suggest: change the title to describe "what is changing in this PR, not just fix John Doe", for example, "check count arguments as default"

plugins/a11y/admin/tests/index.spec.ts Outdated
@@ -45,9 +45,9 @@ before(async () => {

describe('Admin Commands', () => {
it('user/authorize', async () => {
await client1.shouldReply('authorize', '请指定目标用户。')
// await client1.shouldReply('authorize', '请指定目标用户。')
Copy link
Member

Choose a reason for hiding this comment

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

Why is it commented

plugins/a11y/sudo/src/index.ts Outdated
Comment on lines 19 to 21
if (!message) return session.text('.expect-command')

Copy link
Member

Choose a reason for hiding this comment

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

Why to delete these lines? I think it would be better to show this message instead of the default one that didn't explicitly notify which argument it lacks.
Instead, what about to register this command with checkArgCount: false?
Also, others commands everywhere that has the same problem as well.

@BH4HPA BH4HPA closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaikoTan MaikoTan MaikoTan left review comments

@shigma shigma Awaiting requested review from shigma

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: 应令 true 成为指令配置中 checkArgCount 的缺省值。
3 participants
@BH4HPA @MaikoTan @shigma