deprecate replace(s::String, pat, r, n) to replace(s, pat=>r, count=n)

[rfourquet]

This is a follow-up of #22324, to have a consistent replace. This does not try to add replace(s, pat1=>r1, pat2=>r2, ...) as this can be added any time later.
EDIT: this is based on that PR, so only one new commit here!

[ararslan]

Won't this be nothing when pred(x) is false? That seems oddly implicit. IMO it would be clearer to write it as x->pred(x) ? Some(new) : nothing if that is indeed the intent.

[rfourquet]

I find lovely this implicit nothing, but I appreciate many will disagree. I was wondering if this is a pattern which will emerge, to pune on the nothing return value from if without else and for. I remember that you precisely would have prefered something else than Void to replace Nullable... I'm absolutely fine to change this here, but will do next time there is a more serious reason to push (CI resources are scarces, though not as much as on deadline day ;-) )

[StefanKarpinski]

I don't think it matters much and it's kind of cute that this works, but I have to say I do prefer the explicitness of pred(x) ? Some(new) : nothing. The if version requires the reader to know that if without and else evaluates to nothing when the condition is false.

[rfourquet]

If anywone wonders, the errors come from that I forgot to update stdlib code. I will update here if/when the other replace PR gets merged.

[Ismael-VC]

Why pairs and not keyword arguments?

[StefanKarpinski]

Because the LHS of a keyword argument can only be a symbol?

[rfourquet]

Why pairs and not keyword arguments?

It didn't occur to me, but my "feeling" is that pairs are much better suited for this. I see more keyword args as a way to tweak the algorithm implemented by the function rather that providing data. Or at least, the data would be the value of the the the kwarg, and the key part should be quite static, and documented.

[Ismael-VC]

Because the LHS of a keyword argument can only be a symbol?

Oh nice, now I get it, thanks!

Could this be a good opportunity to choose a more mnemonic name for the r argument, how about repl for replacement? It seems to be a used abbreviation in dictionaries.

[ararslan]

The name of the argument isn't really user-facing, so I don't see why it matters what's used.

[Ismael-VC]

I don't agree, method signatures are user-facing and documentation too (when there are no docs all you get is the method signature).

If you query for it's docstring this is the signature replace(s::AbstractString, pat, r, [count::Integer]), luckily in this case, the docstring explains what r means (I'd be great if we stopped assuming it's pretty obvious what variables like r means, even with context, even with docstrings):

help?> replace("foofoofoo", r"foo", "bar", 2)
  replace(s::AbstractString, pat, r, [count::Integer])

  Search for the given pattern pat in s, and replace each occurrence with r. If count is provided, replace at most count occurrences. As with search, the second
  argument may be a single character, a vector or a set of characters, a string, or a regular expression. If r is a function, each occurrence is replaced with
  r(s) where s is the matched substring. If pat is a regular expression and r is a SubstitutionString, then capture group references in r are replaced with the
  corresponding matched text. To remove instances of pat from string, set r to the empty String ("").

s::AbstractString could be str::AbstractString, here at least it has a type annotation, so it's easy to tell, but variables like this are a bad idea for the Core/Base/stdlib language, they are cool for peoples own code I guess, but not for the core language API of Julia IMHO. I think it's ok if those are private local variables (a closure could get access to those, so not entirely private), but method argument names and parameters are part of the public API.

Here however we can see the docstring and the actual name of the argument are out of sync, here it is f for function, since replacement can be either a string or a function and this is user-facing:

julia> @which replace("foofoofoo", r"foo", "bar", 2)
replace(s::AbstractString, pat, f, n::Integer) in Base at deprecated.jl:1345

julia>

At least the docstring and the documented argument should be named the same. Some better wording would be great too IMHO, ie:

Search for the given pattern pat in s, and replace each occurrence with r.
vs
Search for the given pattern pat in the string s, and replace each occurrence with the replacement r.

[StefanKarpinski]

Positional argument names can be changed at any point without breaking anyone's code. We are at the moment focused on things that need to be changed for 1.0. We can bikeshed better positional argument names until the cows come home post 1.0.

[Ismael-VC]

I see, I'll make an issue and open a PR for bad keyword argument names, since those would be breaking, hope there are none. :)

[StefanKarpinski]

Triage says please do this!