Wouldn't it be better to call delete so it would match Ruby behaviour?

$ ruby -e "puts '12'.tr('1','')"

2

It makes it better to use than having to deal with the exception.

I'm thinking sometimes the parameter might come from user input or
some other source which the developer didn't anticipate,

having it raise seems less dev friendly.

On Tue, Aug 9, 2016 at 9:57 PM, Patrik Wenger notifications@github.com
wrote:

---

You can view, comment on, or merge this pull request online at:

#3129
Commit Summary

• String#tr: don't crash if replacement is empty
• fix typo
• String#tr(_, ""): raise earlier and improve message

File Changes

• M spec/std/string_spec.cr
  https://github.com/crystal-lang/crystal/pull/3129/files#diff-0 (32)
• M src/compiler/crystal/codegen/const.cr
  https://github.com/crystal-lang/crystal/pull/3129/files#diff-1 (2)
• M src/string.cr
  https://github.com/crystal-lang/crystal/pull/3129/files#diff-2 (1)

Patch Links:

• https://github.com/crystal-lang/crystal/pull/3129.patch
• https://github.com/crystal-lang/crystal/pull/3129.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3129, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD66NDh_u9R-1eH6yzpNXKH3K76XTOvks5qeOm-gaJpZM4Jgg1d
.

You're right. Didn't think that far. 😅 I'll improve it.

What about now? :)

I'm not sure I agree, I never used tr with user input the parameters. I think .tr(whatever, '') is actually misuse of tr, as delete reveals the intent a whole lot better and IME it happens out of ignorance or lack of knowledge for/of String#delete.

I've seen a couple of times Ruby code like "xpto".gsub(/xp/,var) then
for some reason gsub is no longer fast enough and someone decides to
replace it with .tr

It's safe to assume then that you expect it to work for any value of var,
if for the special case where var="" or nil it raises then IHMO it's not
the most intuitive outcome.

On Tue, Aug 9, 2016 at 10:18 PM, Jonne Haß notifications@github.com wrote:

I'm not sure I agree, I never used tr with user input the parameters. I
think .tr(whatever, '') is actually misuse of tr, as delete reveals the
intent a whole lot better and IME it happens out of ignorance or lack of
knowledge for/of String#delete.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3129 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD66Mzf_C8BmW7Ih-AF0oMzFzMydX2Oks5qeO60gaJpZM4Jgg1d
.

https://github.com/search?q=gsub&ref=simplesearch&type=Code&utf8=%E2%9C%93

Searching for .tr is too hard on github since search doesn't support . chars

On Tue, Aug 9, 2016 at 10:36 PM, Luis Landeiro Ribeiro <
ribeiro.luis@gmail.com> wrote:

I've seen a couple of times Ruby code like "xpto".gsub(/xp/,var)
then for some reason gsub is no longer fast enough and someone decides to
replace it with .tr

It's safe to assume then that you expect it to work for any value of var,
if for the special case where var="" or nil it raises then IHMO it's not
the most intuitive outcome.

On Tue, Aug 9, 2016 at 10:18 PM, Jonne Haß notifications@github.com
wrote:

I'm not sure I agree, I never used tr with user input the parameters. I
think .tr(whatever, '') is actually misuse of tr, as delete reveals the
intent a whole lot better and IME it happens out of ignorance or lack of
knowledge for/of String#delete.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#3129 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAD66Mzf_C8BmW7Ih-AF0oMzFzMydX2Oks5qeO60gaJpZM4Jgg1d
.

Uhm, .gsub(/xp/, var) and .tr("xp", var) have very different behavior.

It boils down to:

1. Just make it work like #delete in case the replacement is empty.
   • pro: Just Works. (Principle Of Least Surprise, Ruby people will feel comfortable)
   • contra: It could hide misuse of String#tr.
2. Raise ArgumentError to signal misuse.
   • pro: Actual intent is revealed a whole lot better.
   • contra: Doesn't Just Work.
   • pro: Reveals misuse of String#tr in case one accidentally specifies an empty string or more severe problems with code, if, for some insane reason, user input is directly passed in. (The latter would only be detected through something like fuzzy testing, or when it's too late anyway.)

I don't really have a strong opinion. The Ruby world deals with (1). Granted, (2) does seem cleaner/safer.

I'm also hesitant about either route, but using some examples:

"hello".tr("el", "ay") # replace 'e' with 'a' and 'l' with 'y'
"hello".tr("el", "a") # replace 'e' and 'l' with 'a'
"hello".tr("el", "") # replace 'e' and 'l' with nothing, so delete them

I think it doesn't hurt if we simply delegate to delete.

Side note: Ruby's tr apparently is much more complex, supporting character ranges and negating sets. I can see we have that implemented in count, delete and squeeze, should we maybe do it for tr too? I'm not very strong on this, I personally don't see a lot of use for tr where gsub can be used.

Like I said, I don't have a strong opinion. I also don't have a lot of experience with Crystal yet, so no intuition on how this ought to be solved.

Character ranges and negating sets are indeed missing and could be a nice Crystal programming exercise. It'd most likely be more performant/have a smaller footprint than using gsub since that uses regular expressions.

Just a side note here: damn how nice it would be if things like this could error at compile time when literals are used (can via macro wrapping). Well, anyways...

Out of a psychological perspective, perhaps defining result for any input would make a user more aware of covering odd cases. And dub these as "bad form" in the docs (ie: "worse style than using unsafe_at" ;-) ). Provided it doesn't impact performance more than negligibly!

Like:

• empty left = nop
• empty right = delete
• fewer letters on right = repeat last letter to match size
• fewer letter on left = discard excess on right

In an utopian world it should of course be very simple.

I like @ozra's suggestion about erroring at compile-time if to is an empty string literal and just behaving like #delete if it's a variable that happens to be an empty string. As an exercise, I tried implementing this in a test class Str using macros but haven't been successful yet:

class Str
  getter string
  def initialize(@string : String)
  end

  def tr(from : String, to : String)
    {% if to.is_a?(StringLiteral) && to.empty? %}
      {{raise}}
    {% else %}
      @string.tr(from, to)
    {% end %}
  end
end

s = Str.new "Foo"
p s.tr("o", "i")
p s.tr("o", "")

Unfortunately I don't have access to to in this way. Wrapping the whole thing into macro tr(from, to) makes it impossible to call on a Str object. Are macros only to be called within class definitions?

I like @ozra's suggestion about erroring at compile-time if to is an empty string literal and just behaving like #delete if it's a variable that happens to be an empty string.

i think that's the best of both worlds if we can pull it off. If not, i like the defaulting to calling delete if it's empty.

Really, I think making tr a macro just for this is completely over-engineering the solution to this problem. If people want to use tr with an empty string instead of delete let them, it's their own code.

@paddor Thank you!

I merged because of #3129 (comment) and because I agree with @RX14, we are over-discussing this.

As a side note, you can't currently have macros on class instances, only on classes.