-
-
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
Implement Char#*(times) operator #3860
Conversation
def *(times : Int) | ||
raise ArgumentError.new "negative argument" if times < 0 | ||
|
||
if times == 0 || bytesize == 0 |
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.
bytesize == 0
is impossible
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.
See my previous comment.
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.
@RX14 I was still thinking of String
, my bad - thanks for pointing that out! just fix-ed it ™
# ``` | ||
# '-' * 10 # => "----------" | ||
# ``` | ||
def *(times : Int) |
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.
Call times
count
internally?
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.
Implementation is taken straight from String#*
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 would prefer to change both here and at String#*
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 changed it in both, String#*
and Char#*
methods.
LGTM |
e15ba58
to
cae18ee
Compare
# ``` | ||
# '-' * 10 # => "----------" | ||
# ``` | ||
def *(times count : Int) |
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.
@Sija why named arguments in an infix operator?
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.
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.
Sorry for joining late :-), but it's hard to imagine someone will write "aString".*(times=3)
.
I am ok with either name, but I see no point with the named argument in an infix operator.
Unless there is a reason I would drop that. Again any internal name is ok.
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.
Fair point, although I was thinking about the docs, where the argument name is shown.
@@ -2149,19 +2149,19 @@ class String | |||
# "Developers! " * 4 | |||
# # => "Developers! Developers! Developers! Developers!" | |||
# ``` | |||
def *(times : Int) | |||
raise ArgumentError.new "negative argument" if times < 0 | |||
def *(times count : Int) |
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.
ditto.
And in this case, this change is just a param rename. I get the consistency, but, is there any reason to prefer times over count in both cases?
Sorry, I'll have to intervene and say no to this. What's the use case for this? Plus, doing Finally: 'a' + 1 # => b
'a' * 2 # => "aa" Doesn't make sense at all. Plus the code for multiplying a Char and String is almost identical, I wouldn't like to have that in the codebase. |
@asterite Fair 'nuff, I was thinking too about intermixing chars and integers which doesn't makes much sense. Also, we need to allocate |
Comes useful when in need to repeat just one
Char
without reverting toString
.