-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Move *io* as first argument of method overloads #5925
Move *io* as first argument of method overloads #5925
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very very odd.
Optional parameters supposed to come after required arguments. I really don't like:
join(separator)
—allrightjoin(io, separator)
—what?!
@@ -2,11 +2,11 @@ require "spec" | |||
require "big" | |||
|
|||
private def to_s_with_io(num) | |||
String.build { |str| num.to_s(str) } | |||
String.build { |io| num.to_s(io) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change. I also prefer the str
idiom for String.build
. We're building a string, not whatever io.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert that.
But I always find this weird ("why is this passing a String?"): str
is not a string but a string builder (so it would make sense to call it builder
). Which is an IO and io
is shorter but equally expressive. The argument in the method definition of to_s
is also called io
. So it makes very much sense to call this io
.
Maybe injecting the |
@ysbaddaden Could we keep the general discussion about this in #5916, please? |
7ae22bc
to
ca3c657
Compare
216abdd
to
5a1cde2
Compare
@straight-shoota You meant #5916? |
Closing. There has been no resolution on this and the PR would need a huge rebase, so it would be easier to start fresh. |
Fixes #5916