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

Fix 2D6 on SwordWorld2.0 #192

Merged
merged 2 commits into from
May 15, 2020
Merged

Fix 2D6 on SwordWorld2.0 #192

merged 2 commits into from
May 15, 2020

Conversation

ysakasin
Copy link
Member

DiceBot#check2dCriticalのcriticalにnilが渡されていたせいで、SwordWorld2.0で2D6が判定できなかった。DiceBot#check2dCriticalをcriticalがnilでない時に呼び出すようにして解決する。

Fix #191

@codecov-io
Copy link

Codecov Report

Merging #192 into master will decrease coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
- Coverage   87.20%   86.78%   -0.43%     
==========================================
  Files         198      208      +10     
  Lines       20687    21710    +1023     
==========================================
+ Hits        18040    18840     +800     
- Misses       2647     2870     +223     
Impacted Files Coverage Δ
src/dice/add_dice/randomizer.rb 96.00% <100.00%> (ø)
src/configBcDice.rb 100.00% <0.00%> (ø)
src/test/range_table_test.rb 100.00% <0.00%> (ø)
src/test/testDiceBotPrefixesCompatibility.rb 88.23% <0.00%> (ø)
src/test/test_dicebot_info_is_defined.rb 100.00% <0.00%> (ø)
src/diceBot/Cthulhu_ChineseTraditional.rb 14.41% <0.00%> (ø)
src/test/test_d66_table.rb 100.00% <0.00%> (ø)
src/diceBot/Cthulhu7th_ChineseTraditional.rb 15.10% <0.00%> (ø)
src/test/test_detailed_rand_results.rb 100.00% <0.00%> (ø)
src/test/testDiceBotLoaders.rb 99.78% <0.00%> (ø)
... and 11 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 b0be3d7...8d76dd2. Read the comment docs.

@ochaochaocha3
Copy link
Member

ochaochaocha3 commented May 15, 2020

ソード・ワールド側にもクリティカル値を判定する処理が含まれているので、判定処理が分散しないように、ソード・ワールド側で対応する方が良いと思います(return if !critical || critical <= 2)。
https://github.com/bcdice/BCDice/blob/v2.05.00/src/diceBot/SwordWorld2_0.rb#L146

また、DiceBotとSwordWorld2_0の両方のcheck2dCriticalにYARDコメントでパラメータの型を書いておくと、ミスが起こりにくくなりそうです。

@ochaochaocha3
Copy link
Member

ソード・ワールド固有のメソッドのインターフェースをDiceBotに持たせているのは苦肉の策でしょうから、将来的にはソード・ワールド側に完全移行できると良いですね。

Copy link
Member

@ochaochaocha3 ochaochaocha3 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ysakasin ysakasin merged commit 9ab70b0 into master May 15, 2020
@ysakasin ysakasin deleted the fix_sw_2d6 branch May 15, 2020 17:30
ysakasin added a commit that referenced this pull request Aug 30, 2020
* Fix 2D6 on SwordWorld2.0

Fix #191

* オーバーライドした側で弾く
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.

SwordWorld2.0の2d6でエラー
3 participants