Issue 9699 - strip functions should allow setting custom match character.

[ghost]

http://d.puremagic.com/issues/show_bug.cgi?id=9699

[JakobOvrum]

These should probably use predicates instead of hard-coded values.
I think it's safe to rely on inlining of template arguments.

@andralex, any word on such inlining?

[ghost]

I've added predicate overloads.

[JakobOvrum]

LGTM

[jmdavis]

And what does this buy us over simply using find?

[ghost]

And what does this buy us over simply using find?

You'll have to demonstrate how find is a good replacement for these convenient functions. Several people have asked why the strip family of functions don't accept setting a custom match character, so I finally made a pull.

[JakobOvrum]

Code like:

auto stripped = "\n test\t".strip!isWhite();

Is not at all pretty with find; left-stripping is trivial, right-stripping is a mess,
and doing both at the same time is a chore, as far as I can see.

[jmdavis]

auto left = str.find!(a => a != delim)();
auto right = str.retro().find!(a => a != delim)().retro();
auto both = right.find!(a => a != delim)();

Having the strip versions is somewhat cleaner, but it doesn't add any functionality.

[ghost]

Those return a lazy range, it's completely different. And calling array on that is expensive.

[jmdavis]

Nope. They return string.

[jmdavis]

These may be worth adding from the standpoint of user friendliness, but they don't add any functionality, which is why I never implemented them when someone brought them up in the past.

[CyberShadow]

I think the predicate should also accept strings in addition to delegates. The functions in std.algorithm / std.range use some fancy wrapper that turns mixins to delegates or something like that.

These may be worth adding from the standpoint of user friendliness, but they don't add any functionality, which is why I never implemented them when someone brought them up in the past.

How do you define what "adds functionality"?

[JakobOvrum]

Having the strip versions is somewhat cleaner, but it doesn't add any functionality.

... somewhat?

It obviously doesn't add anything that can't be done with std.algorithm and std.range, but that is the case
with most of this module already, so I don't see it as a good point.

[alexrp]

The functions in std.algorithm / std.range use some fancy wrapper that turns mixins to delegates or something like that.

Is there a good reason to do this? We're largely working towards phasing string lambdas out and using proper lambda expressions instead. I don't think there's a good reason to proliferate string lambdas any longer.

[CyberShadow]

No idea. You're probably right. I thought it was the convention to use them.

[JakobOvrum]

I think the predicate should also accept strings in addition to delegates. The functions in std.algorithm / std.range use some fancy wrapper that turns mixins to delegates or something like that.

std.string already imports std.functional which has unaryFun which is the relevant one,
so it should be easy to plug.

However, I'm not sure if supporting the string syntax is a good idea. It was born when the lambda syntax
was not available and I think it's generally agreed upon that the lambda syntax is mostly superior.

The one benefit of the string syntax is that it is slightly shorter - but it's only shorter because
it implicitly introduces one or two variables with the fixed names a and b, which I don't think
is nearly as good as the explicitness of the lambda syntax. We can't easily remove the option from
existing interfaces because it breaks code, but we can easily not propagate it any further.

[ghost]

Nope. They return string.

That auto inference tricked me.

Anyway how can you be advocating this:

string str = "xx abc xx";
string both = str.retro().find!(a => a != 'x')().retro().find!(a => a != 'x')();

over this:

string str = "xx abc xx";
string both = str.strip!(a => a == x);

[jmdavis]

Anyway how can you be advocating this:

My point is that you can do what you're trying to do here just fine with what we already have. New functions aren't necessary. They may ultimately be desirable on the grounds that the equivalent of stripRight is a bit ugly and that it would be more user-friendly to just add these, but at that point, it's a question of user friendliness, not functionality. And if we had rfind (or whatever we'd decide to call it), then you wouldn't have all of the retro ugliness, making this it so that this adds even less.

So, if we decide that it's worth adding on the grounds of user-friendliness, fine. But it's not adding anything that we can't already do in std.algorithm. And Andrei at least has typically shot down stuff on those grounds (though I don't think that I feel anywhere near as strongly about that as he does, and I have no idea what he'd think in this case). There's also the question of whether this should be made more general (e.g. put in std.array to work on all arrays, since these have nothing to do with whitespace), but as soon as you start going down that route, pretty soon, the question becomes whether they should work on all ranges, in which case, find already does this, making adding something like this to std.algorithm for general ranges almost completely redundant.

As for the predicates, I think that all predicates should support the string syntax. It's often cleaner than lambda literals, and there's no reason not to support it as far as I can see, unless you're trying to get rid of string lambdas, which I would be very much opposed to. Not supporting them is inconsistent, and it's trivial to add support. Both types of lambdas should work, and then the user can use whichever is best for their code.

[ghost]

My point is that you can do what you're trying to do here just fine with what we already have.

That point is completely lost to anyone who used strip functions and wanted to pass a character. So now you're saying the user has to write half a dozen expressions and reverse the predicate.

If you really want to advocate writing excessively long and unreadable code then go ahead and remove half the stuff in std.string, because you can clearly use existing functions like find instead of strip.

Every time we try to improve the API this pointless discussion comes up, as if someone said at some point "Phobos development is now frozen, user-convenience is a non-issue". It's absurd that adding breaking language changes and language enhancements is now easier to do than adding convenient functions like these to Phobos.

As for the predicates, I think that all predicates should support the string syntax. It's often cleaner than lambda literals, and there's no reason not to support it as far as I can see

See Walter's last two comments here.

There's no need to bring it back from the dead.

[jmdavis]

Every time we try to improve the API this pointless discussion comes up, as if someone said at some point "Phobos development is now frozen, user-convenience is a non-issue". It's absurd that adding breaking language changes and language enhancements is now easier to do than adding convenient functions like these to Phobos.

Walter and Andrei both seem to be big on not making Phobos shallow, which tends to mean not adding functions which do what other functions already do. To some extent, I agree, and to some extent I disagree - usability does trump the goal of making the library deep at least some of the time. But any function which just adds convenience needs to be discussed, or we'll just end up with a pile of convenience functions which do what other functions already do but slightly differently.

I brought up the fact that find will do what these additions do, because it means that these are merely convenience functions, which means that the question of whether they should be added comes down to whether the additional convenience that they add is worth it and not whether they add new functionality. If the consensus is that it is worth it, then fine.

See Walter's last two comments here.
There's no need to bring it back from the dead.

There's a big difference between not using it in examples and killing it off, and not supporting it here just makes it inconsistent with the rest of Phobos. I'd strongly argue that we either get rid of string lambdas completely (which I'm opposed to) or that all predicates in Phobos support them.

[andralex]

Here are my thoughts on the subject:

• stripRight and strip with element or predicate are useful for strings and awkward to achieve with primitives in std.algorithm. (At the same time strip* without element or predicate (using implicitly isWhite) remain specific to strings as there's no general notion of "whitespace".)
• They can be easily generalized and hoisted into std.algorithm, so the question becomes whether they're useful outside the strings realm. I can imagine they can be used to strip zeroes or negative numbers etc. out of a range of numbers.
• For completeness, we'd also need to add stripLeft although it's a call to find followed by assignment. Otherwise, "why stripRight and strip but no stripLeft" becomes an FAQ.

[JakobOvrum]

Both types of lambdas should work, and then the user can use whichever is best for their code.

I think the string predicates are just plain harmful.

• They don't get syntax highlighting unless q{} is used, which nobody seems to do and might be less syntactically appealing. This makes them harder to identify as code and not data.
• The set of symbols available inside the predicate is determined by Phobos internals.
• They produce worse error messages.
• They implicitly introduce variables.
• It's one more concept to learn for readers of D code. It's superfluous.

The new lambda syntax has none of the above problems. Let's not proliferate string predicates unnecessarily.

[andralex]

Oh, regarding lambdas - I think we should slowly phase out string lambdas.

[ghost]

@andralex

They can be easily generalized and hoisted into std.algorithm, so the question becomes whether they're useful outside the strings realm. I can imagine they can be used to strip zeroes or negative numbers etc. out of a range of numbers.

Right, but people expect code which uses strip and imports std.string to continue to work. Perhaps we should simply introduce overloads in std.algorithm or std.array which work for types other than string? Anyway I think this should then be a separate pull request.

[andralex]

std.string should import from std.algorithm appropriately.

[andralex]

I think there should be only one implementation, possibly with some satellite alias definition or (worst case) forwarding functions.

[jmdavis]

@andralex Honestly, I wouldn't think that stripping a specific element from either side of a range is likely to be used much outside of strings. For other ranges, I would expect it to be frequent to strip elements which match a predicate, but I'd also think that strip is a weird name for that. A more appropriate name for that would be dropWhile, which you rejected previously on the grounds that it was just find with its predicate reversed.

It won't be the end of the world if we end up with generic strip, stripLeft, and stripRight functions in std.algorithm which either take an element or a predicate, but it would be a bit odd IMHO.

[andralex]

@jmdavis If it's not much extra work and is not conducive to unexpected errors and bugs it's probably worth just letting strip work with whatever, on grounds of the TAWD (Turtles All the Way Down) principle alone. Even beyond TAWD, I can think of applications to non-standard character types, doubles (strip the audio of all leading and trailing zeros) etc. Since there's a notion of strip it's much better to work that out instead of dropWhile, not to mention strip has left/right/both versions. @AndrejMitrovic thoughts?

[ghost]

@andralex: That's a cute thing, striping silence in audio. So we want the equivalent of this pull request except the strip function would take a range and its element type as arguments?

[andralex]

@AndrejMitrovic yah, and they should go in std.algorithm. The strip function with no element and no predicate is in std.string and strips whitespace.

[ghost]

But I guess they should be called stripFront and stripBack if they work with ranges? And perhaps introduce stripLeft/stripRight as aliases of them into std.array?

[jmdavis]

@andralex dropWhile is more general, so I'd tend to favor it over strip, but all this gets a bit murky, since it gets entertaining to strike the appropriate balance between not duplicating behavior unnecessarily and having the functions be appropriately user friendly. So, if you're okay with this, then we can move forward with it as you suggested.

@AndrejMitrovic I'd probably just keep them as stripLeft and stripRight, since we already have those, and it avoids creating aliases, but it does fit in with the range naming scheme to have stripFront and stripBack.

[andralex]

@AndrejMitrovic do you plan to finish this? Please advise, thanks.

[ghost]

@andralex: Yes, today.