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

迷宮キングダム:基本ルールブック、上級ルールブック記載の表を追加 #218

Merged
merged 13 commits into from
Jul 11, 2020

Conversation

Nyandlion
Copy link
Contributor

迷宮キングダム:基本ルールブック、上級ルールブック記載の表を追加しました。
表追加に伴い、テストデータも追加しました。
誤字を修正しました。
能力値による判定の表記ぶれを修正しました。
天空部屋特殊遭遇表(SEN)の追加に伴い、特殊遭遇表(SE)との競合を避けるため、特殊遭遇表のコマンドを「ENC」に変更しました。

色々追加したので、変更箇所多めです……。

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2020

Codecov Report

Merging #218 into master will increase coverage by 0.71%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   86.93%   87.65%   +0.71%     
==========================================
  Files         210      216       +6     
  Lines       22059    22771     +712     
==========================================
+ Hits        19178    19960     +782     
+ Misses       2881     2811      -70     
Impacted Files Coverage Δ
src/diceBot/MeikyuKingdomBasic.rb 91.98% <ø> (-8.02%) ⬇️
src/test/testDiceBotLoaders.rb 99.78% <0.00%> (-0.01%) ⬇️
src/configBcDice.rb 100.00% <0.00%> (ø)
src/diceBot/OracleEngine.rb 93.93% <0.00%> (ø)
src/utils/format.rb 100.00% <0.00%> (ø)
src/test/test_command_parser.rb 100.00% <0.00%> (ø)
src/utils/command_parser.rb 96.38% <0.00%> (ø)
src/diceBot/NeverCloud.rb 97.87% <0.00%> (ø)
src/diceBot/SamsaraBallad.rb 97.67% <0.00%> (ø)
... and 5 more

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 0cfb903...0c34553. Read the comment docs.

Copy link
Member

@ysakasin ysakasin left a comment

Choose a reason for hiding this comment

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

表の記述にはTableクラスを使ってください。コードの下部で使われています。

使い方が把握できなければこちらで直します。

@ysakasin ysakasin self-requested a review June 16, 2020 03:23
Copy link
Member

@ysakasin ysakasin left a comment

Choose a reason for hiding this comment

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

表の記述にはTableクラスを使ってください。コードの下部で使われています。

使い方が把握できなければこちらで直します。

@ysakasin
Copy link
Member

あと、今まであったテストケースがいくつか消されているので、消さずに元に戻してください。

@ysakasin
Copy link
Member

五月雨式にすみません。変更が多く、誤字修正と追加分の区別がつかないので、誤字修正を別のPull Requestにしていただけませんか?

@Nyandlion
Copy link
Contributor Author

誤字と表記ぶれ修正を別のPull Requestにしました。

Tableクラスの件ですが、Tableクラスで作成できるものは全てTableクラスで記述しています。
D66表の作成や、表内で別の表を参照したい場合の記述方法が分からず、そのまま書きこむ形にしています。記載方法を教えていただけないでしょうか?

@ysakasin
Copy link
Member

別PRの作成ありがとうございます。

D66系は D66Table (src/utils/d66_table.rb) と D66GridTable (src/utils/d66_grid_table.rb) を使ってください

複数テーブルの中からいずれかを実行する系のコマンドは、上記のインスタンスを定数から取り出して D66Table#roll を実行すれば大丈夫です。

文言の中で別表を参照するタイプはTable側に別途処理が必要そうなので、いったんそのままにしておいてください。こちらで直します。MeikyuKingdomのメソッドをオーバーライドしているものは、Tableクラスにせずに、そのままでOKです。

@Nyandlion
Copy link
Contributor Author

ありがとうございます。こちらでTableクラスに直してみます!

@ysakasin ysakasin added the enhance dicebot ダイスボットへの機能追加 label Jun 24, 2020
@ysakasin
Copy link
Member

ysakasin commented Jul 7, 2020

@Nyandlion 既存テーブルの改善の変更が別PRでマスターにマージされたためコンフリクトしてしまいましたが、解消できそうでしょうか? 無理そうなら、こちらで作業しますがどうしますか?

@ysakasin
Copy link
Member

ysakasin commented Jul 8, 2020

@Nyandlion SENSE は混同しやすいものの、実行できなくなるような衝突ではありません。SEを削除してしまうとコマンドの後方互換性が損なわれることになり、これは避けたいです。提案してくださった ENCSE のどっちでも実行できるようにするのが良いかと思いますがどうでしょう。

また、ヘルプメッセージにどちらでも実行可能なことは記載したいです。

@Nyandlion
Copy link
Contributor Author

@ysakasin 元々どどんとふ用に作っていたものだったので、SENとSEは衝突すると思い、変更していました。
現在のTableで衝突を起こさないのであれば、SEのままでいいと思います。

@ysakasin
Copy link
Member

ysakasin commented Jul 10, 2020

@Nyandlion なるほど…… 無理やり String#slice! するのは得策ではないので、 e59c105 はRevertして、 f0ee164 から SE だけ直してマージしようと思います。

@Nyandlion
Copy link
Contributor Author

@ysakasin 承知しました。お任せします。

@ysakasin ysakasin merged commit 49dc73b into bcdice:master Jul 11, 2020
@ysakasin
Copy link
Member

@Nyandlion PRありがとうございました。途中、右往左往が多くてすみません 🙇

ysakasin added a commit that referenced this pull request Aug 30, 2020
* 迷宮キングダム:基本ルールブック、上級ルールブック記載の表を追加

* 迷宮キングダム:基本ルールブック、上級ルールブック記載の表を追加

* カップル休憩表が2つあったので1つ削除

* コード再整形、整形後に発生するエラーを手直し

* randを使っていた部分をrollに変更

* テストケースを元に戻し、追加分を下に追記

* 別PRに分けたので、追加した表を下にずらしました

* コンフリクト解消中に行ズレを起こしていたので修正しました

* 出来る限りTableクラスに書き変えました。ENCをSEに戻しました。

* Revert "出来る限りTableクラスに書き変えました。ENCをSEに戻しました。"

This reverts commit a7092b0ca7cbe16e5281140d24edf391b72a1333.

* ENC -> SE

Co-authored-by: SAKATA Sinji <[email protected]>
@Nyandlion Nyandlion deleted the mayokin_advance branch December 29, 2020 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance dicebot ダイスボットへの機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants