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

Rational#to_i のサンプルを改良 #1627

Open
wants to merge 3 commits into
base: master
from

Conversation

@scivola
Copy link
Contributor

scivola commented Nov 25, 2018

元の Rational(-30, 2) だと,そもそも整数なので切り捨て効果が分かりにくく,また負数の場合にどちらに丸められるのかが例からは分かりません。

そこで,floor, ceil, round でも採用されている Rational(-3, 2) に取り換えました。

scivola
元の Rational(-30, 2) 
だと,そもそも整数なので切り捨て効果が分かりにくく,また負数の場合にどちらに丸められるのかが例からは分かりません。

そこで,floor, ceil, round でも採用されている Rational(-3, 2) に取り換えました。
@sho-h

This comment has been minimized.

Copy link
Member

sho-h commented Dec 23, 2018

rdoc でも同じ行に修正が入ってますけど、 Rational(-31, 2).to_i ですね。-16か-15かはわかるので、rdocに合わせるのはどうでしょう?

@scivola

This comment has been minimized.

Copy link
Contributor Author

scivola commented Dec 24, 2018

round, floor, ceil, truncate は仲間なのでこの四メソッド間でなるべくサンプルを共通化するのがよさそうに思ったのですが,いかがでしょうか。
(truncate と to_i はエイリアスの関係にはありませんが,引数無しの場合は同じですよね?)

@sho-h

This comment has been minimized.

Copy link
Member

sho-h commented Dec 24, 2018

先にrdocを直してもらえるならいいのではないかと思います。ruby/rubyにPR出してみてもらってもいいですか?

@scivola

This comment has been minimized.

Copy link
Contributor Author

scivola commented Dec 24, 2018

え゙ っ,それはちょっとハードルが高いですね……
それならばプルリクエストを変更するほうを選びます。
のちほど作業します。

@sho-h

This comment has been minimized.

Copy link
Member

sho-h commented Dec 31, 2018

あら、本体の方にもIssue挙げられたりしてなかったでしたっけ...すみません、その点は勘違いだったかも...

@scivola

This comment has been minimized.

Copy link
Contributor Author

scivola commented Jan 27, 2020

Rational(-3, 2) と提案していたのを rdoc に合わせて Rational(-31, 2).to_i に差し替えました。
(ruby/ruby に PR 出すのはハードルが高かったため)

refm/api/src/_builtin/Rational Outdated Show resolved Hide resolved
あ,ほんとだ

Co-Authored-By: Seiei Miyagi <hanachin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.