crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.47k stars 1.62k forks source link

Implement String#remove #3033

Open Sija opened 8 years ago

Sija commented 8 years ago

It's rather handy replacement for String#gsub(/.../, ""). Ruby has it AFAIR since v2.0.

asterite commented 8 years ago
  1. We have String#delete
  2. Ruby's String#delete doesn't seem to accept Regex as an argument

So... I don't understand what you are exactly requesting.

Sija commented 8 years ago

Late night hours are getting to me, it was String#remove extension that I've been thinking of.

Sija commented 8 years ago

Perhaps it's more apt to leave it for ActiveSupport...

jhass commented 8 years ago

If anything we should add a String#delete overload that takes a regex.

Sija commented 8 years ago

@jhass I think that might create some confusion around String#delete since it takes character ranges instead of strings to remove, which is the case for passing a Regex. Also, that wouldn't be consistent with Ruby's String API.

straight-shoota commented 3 years ago

IMO String#gsub is good enough. It's not as expressive as #remove, but it doesn't add confusion about the difference between #delete and #remove.

Maybe a useful enhancement could be to make the replacement argument default to nil/"", so you could do: "foo bar baz".gsub("bar").

In my ideal API world, all #delete methods would probably be #tr or #delete_chars and thus open the way for a string/pattern based #remove. Maybe that could be up for consideration, but I'm not sure it's worth breaking this stuff. Although I also don't expect #delete methods to be used much. But that's hard to prove.

Sija commented 3 years ago

Maybe we could add String#delete overloads, taking String and Regex arguments, following @jhass' suggestion.

asterite commented 3 years ago

I think String#delete is really bad, it should be named delete_sets or something like that. Then we could have String#delete to do what everyone would expect it to do.

asterite commented 3 years ago

In addition to what I said, even though the current delete might be useful, I never used it in my life, and maybe it's a niche thing. In contrast, I "deleted" substrings or regex matches from a string a lot of times. So I think we should have both methods (maybe only because we already have one implemented) but we should use the name delete for the most common use case, and use a different name for the less common use case.

Ideally this would happen before 1.0. And since I don't think String#delete is used much, maybe it could be done without a deprecation notice (which is also kind of impossible to do).

straight-shoota commented 3 years ago

I too have never used and only been confused by String#delete. 😄

Deprecating is possible, but for a clean transition requires three releases: 1. deprecate, 2. remove, 3. re-introduce.

jhass commented 3 years ago

Well, String#delete is the way it is because it ports Ruby's :) And I quite regularly have to teach people to use .delete(x) instead of .tr(x, '') or .gsub(/[x]/, '') when they want to remove a set of characters.

asterite commented 3 years ago

On the other hand with sub and gsub you can delete the first occurrence, or all occurrences. So maybe the current state is good... except that the existing delete method might be a bit confusing.

straight-shoota commented 3 years ago

That's true, with sub/gsub it's fairly obvious whether it removes one or all. Assuming you're familiar with the concept of sub/gsub. But at least it's not something you need to learn additionally. So adding String#remove is probably not worth it. Instead I'd repeat my proposal to add default values to sub/gsub. Not sure if string.gsub("foo") improves expressiveness much over string.gsub("foo", nil), though.

We could also consider renaming #delete to #delete_chars or other to remove confusion potential when you call that method with a string. The same confusion happens with strip/lstrip/rstrip by the way. Been bitten by that myself 😆 When you pass a string to a method that removes something you would expect that to treat the string as string, not as a collection of characters. A simple improvement to that would be to make chars a named argument only. So you can't accidentally call "foo bar baz".delete("bar") and get the unexpected result of "foo z". Instead you'd have to call it as "foo bar baz".delete(chars: "bar").

stellarpower commented 2 years ago

In any case on this wider discussion, the nil replacement for gsub appears to be undocumented. I think it'd be a good example (deleting a substring or by regex) to add into the docs there. replacement isn't marked nillable or type-restrained, so it's less clear what sorts of things I could pass into it.