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

refactor: スキーマを利用した新コマンドシステム #479

Merged
merged 55 commits into from Sep 17, 2022

Conversation

MikuroXina
Copy link
Collaborator

close #461.

Type of Change:

コードのリファクタ, システムの高度化, テストの修正

Details of implementation (実施内容)

各コマンドが引数のスキーマを定義すると, アダプタ側がそれを読み取ります. コマンドが送られてきたときに, 正しい形式の引数かどうかをスキーマを基に判別して引数の値を取り出します. スキーマの定義は model/command-schema.ts に定義しており, 概ね以下のような構成です.

スキーマ:
  名前リスト
  (サブコマンドが無いときの) 引数リスト:
    - 
      名前
      説明
      デフォルト値など
    - :
    - :
  サブコマンドリスト:
   サブコマンドまたはサブコマンドグループ:
     サブコマンド名:
       引数リスト
       (グループの場合は) サブコマンドリスト

これにより CommandMessage の定義ならびに全てのコマンドの実装を改修し, テストもあわせて修正しています.

Additional Information (追加情報)

この改修に伴い, コマンド実装側で引数の形式や範囲が正しいかどうかチェックする手間がなくなり, コマンド引数の型がより強固になります. その代わり, スキーマの書き方の支援がいまのところ少ない (TypeScript による読みにくいエラーしかない) ので「スキーマの書き方を学ぶ」実装コストが増えています.

コマンド実行の際にスキーマの定義に沿わない引数が渡された場合は, アダプタ側のパース処理の段階でエラーが発生して弾かれます. そのため, パース処理のエラーをユーザーに返信するようになっており, これまでのようなコマンドごとのユニークなエラーメッセージが見られなくなります.

また, コマンドのイベントとメッセージ作成のイベントを分割したため, コマンド側ではメッセージの削除に反応できなくなりました. これにより, 煩わしい event !== 'CREATE' の判定をすべて消しました.

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022

Codecov Report

Merging #479 (ce47c1f) into main (35fbe8d) will increase coverage by 2.66%.
The diff coverage is 78.63%.

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

@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
+ Coverage   71.28%   73.94%   +2.66%     
==========================================
  Files          58       60       +2     
  Lines        3952     4352     +400     
  Branches      373      372       -1     
==========================================
+ Hits         2817     3218     +401     
- Misses        762      763       +1     
+ Partials      373      371       -2     
Flag Coverage Δ
unittests 73.94% <78.63%> (+2.66%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/adaptor/proxy/command/schema.ts 56.84% <56.84%> (ø)
src/model/command-schema.ts 80.84% <80.84%> (ø)
src/service/command/party.ts 81.81% <85.96%> (+5.95%) ⬆️
src/service/command/meme.ts 75.75% <86.95%> (+1.60%) ⬆️
src/service/command/role-rank.ts 88.63% <87.50%> (+9.46%) ⬆️
src/service/command/typo-record.ts 90.60% <88.88%> (+6.03%) ⬆️
src/service/command/kaere.ts 71.19% <89.55%> (+6.60%) ⬆️
src/service/command/guild-info.ts 95.09% <90.00%> (+1.86%) ⬆️
src/service/command/role-create.ts 92.42% <92.30%> (+9.70%) ⬆️
src/service/command/judging.ts 80.36% <92.50%> (+4.90%) ⬆️
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MikuroXina MikuroXina force-pushed the refactor/#461 branch 2 times, most recently from 76d7984 to 0f63050 Compare September 15, 2022 06:29
@MikuroXina MikuroXina enabled auto-merge (squash) September 15, 2022 06:36
Copy link
Contributor

@m1sk9 m1sk9 left a comment

Choose a reason for hiding this comment

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

レビュー前にデバックした際に気がついて、特に #479 などに記述がなかった箇所を書いておきます。

  • !help にてサブコマンドに関する記述が消えてしまっていること
  • feat: userinfoコマンドの拡張案 #474 にて追加された !userinfo me が利用できなくなっていること ( !userinfo me では USER 型の値が期待されているが、 me (おそらく文字列の判定?) になってしまっている)
    Edit(10:41): !userinfo me って想定されていない?

Copy link
Contributor

@m1sk9 m1sk9 left a comment

Choose a reason for hiding this comment

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

お疲れ様です
レビューに自信がないですがよろしくお願いします

src/adaptor/proxy/command.ts Show resolved Hide resolved
src/adaptor/proxy/command.ts Show resolved Hide resolved
src/adaptor/proxy/command.ts Show resolved Hide resolved
src/model/command-schema.ts Outdated Show resolved Hide resolved
src/service/command/kaere.ts Show resolved Hide resolved
src/service/command/party.ts Show resolved Hide resolved
Copy link
Contributor

@m1sk9 m1sk9 left a comment

Choose a reason for hiding this comment

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

OK, approve

@MikuroXina MikuroXina merged commit 586bd15 into main Sep 17, 2022
@MikuroXina MikuroXina deleted the refactor/#461 branch September 17, 2022 04:08
raiga0310 pushed a commit to raiga0310/OreOreBot2 that referenced this pull request Sep 18, 2022
* feat: Add Schema

* docs: Add docs and tidy up

* fix: Add paramsOrder

* fix: Improve SubCommand

* fix: Fix error payload

* fix: Add NEED_MORE_ARGS error

* fix: Add type parameters

* fix: Fix arg of ParsedSubCommand

* fix: Improve parsed types

* fix: Support params on Schema

* refactor: Update CommandMessage

* feat: Impl parseStrings

* fix: Fix to check range

* fix: Add makeError

* fix: Weaken type bound

* test: Add simple test case

* fix: Fix some keys and test case

* fix: Remove parsed key

* test: Enforce test case

* test: Enforce test cases

* fix: Fix DIGITS and update output

* feat: Add MESSAGE type

* fix: Make subCommand and params required

* fix: Weaken type bound

* fix: Improve ParsedSchema

* fix: Fix fallback type

* fix: Fix type on single arg

* fix: Fix type pattern match

* feat: Add ParamBase

* fix: Fix SubCommand definition

* fix: Support variadic arguments

* fix: Make subCommand optional

* feat: Add SCHEMA

* fix: Improve ParamsValues

* fix: Fix registration

* feat: Add parseStringsOrThrow

* fix: Fix type bound

* fix: Improve createMockMessage args

* fix: Remove empty subCommand from output

* fix: Weaken type of param

* fix: Fix on no args but has sub command

* fix: Fix to report error

* test: Update command tests

* fix: Guard on other commands

* fix: Fix typing with Equal

* feat: Improve with CommandRunner

* fix: Remove redundant test

* doc: Update CONTRIBUTING for newer method

* fix: Fix not to throw error

* fix: Fix parsing variadic params

* fix: Fix decoupling

* feat: Add more hibikiness

* fix: Guard with checking editable
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: コマンドに引数スキーマを追加
3 participants