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

TORG EternityのTGコマンドのバグ修正 #242

Merged
merged 2 commits into from
Jul 13, 2020
Merged

Conversation

ysakasin
Copy link
Member

TG12+5TG5と等価になってしまうバグを修正

Input 修正前 修正後
TG12+5 1R20+5 1R20+(12+5)
TG5 1R20+5 1R20+5
TG+5 1R20+5 1R20+5
TG+1+2+3 1R20+3 1R20+(1+2+3)

加えて、不適切な Regexp.last_match の利用を修正

@ysakasin ysakasin requested a review from ochaochaocha3 July 13, 2020 12:16
@ysakasin ysakasin added bug バグってる! refactoring 内部構造の改良 labels Jul 13, 2020
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.

バグ修正については問題ないと思います。

ところで別issue案件ですが、読んでいると以下の点が気になります。TORG本体からの流用のせいだと思いますが…

  • 正規表現冒頭の (^|\s) や末尾の (\s|$) が無駄(括弧で囲む必要がない、空白は strip されるので入らないはず)。
  • parren_killer("(0#{var})").to_i よりも ArithmeticEvaluator.new.eval(var) の方が、意図が明確。

@ysakasin
Copy link
Member Author

@ochaochaocha3 その他の気になる点については、私も同感です。今回はパッチリリースをしたいので、最小限の変更に留めました。

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.

おっと、Approveし忘れていました。修正分は大丈夫だと思います。

@ysakasin
Copy link
Member Author

ありがとうございます!

ここらへんのイケてないかき回しはVer3でまとめて掃除したいですね。

@ysakasin ysakasin merged commit 58dc44d into master Jul 13, 2020
@ysakasin ysakasin deleted the fix_torg_eternity_tg branch July 13, 2020 14:12
ysakasin added a commit that referenced this pull request Aug 30, 2020
TORG EternityのTGコマンドのバグ修正
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug バグってる! refactoring 内部構造の改良
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants