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
strings: NewReplacer does not replace strings in order #25071
Comments
This is working as intended: replacements are being performed in order on the string linearly, without attempting to replace any text that was replaced. The docs should be updated to make this more clear. |
Makes sense. It didn't occur to me to read it the other way. |
Change https://golang.org/cl/109375 mentions this issue: |
If more than one pattern matches at the same target string position (a pattern is prefix of another), the one that is applied does depend on the order they're passed to |
I agree it is hard to be exhaustive without unnecessarily complicating the doc. The proposed change lgtm |
@nicksnyder I'm not sure what I would have done myself, I think it's a pity though. If I want to replace Unless the caller knows in advance that no pattern is prefix of another, I think he would generally prefer a predictable behavior (first pattern first? longest pattern first?). Of course it is also true that if we expose too much details, the docs become less effective for the most common use cases, less clear, and easier to misinterpret. |
I'm not sure what about the current behaviour you find "unpredictable". The patterns are replaced in the order they appear in the string, from left to right. If two (old->new) couples has the same "old", the first couple (in the order they're passed as arguments) is used. I believe this constitutes "predictable" behaviour. Are you suggesting to change something in the implementation? Or to change the documentation? How would you change it? Are you asking to explicitly document the Replacer's behaviour in the "old->new1", "old->new2" case? I think that's a reasonable request. |
Yes, I wondered this myself and had to experiment to find the answer. I found it hard to come up with a pithy doc for it though. |
@ALTree thanks for the reply.
No.
More generally, to explicitly document the behavior of strings.NewReplacer("X1", "X2", "X", "X1").Replace("X1") // "X2"
strings.NewReplacer("X", "X1", "X1", "X2").Replace("X1") // "X11" If I had some good wording to suggest, I would have done so.
Bad choice of word, it probably is predictable, just not explicitly documented. In your own code, would you rely on it, in cases similar to the example above? |
This is already documented, it's the "without overlapping matches" part. "without overlapping matches" means that once we are done with the
Yes, because it's the documented behaviour. |
Incidentally I made an example with possible overlapping matches, but I wasn't referring to them. Here is another example: // first pair is used
strings.NewReplacer("ant", "bug", "antique", "old").Replace("antique") // "bugique"
// pairs swapped, again first pair is used
strings.NewReplacer("antique", "old", "ant", "bug").Replace("antique") // "old" As you said, the first pair is used. |
Ah, I see. Thanks for explaining. So it's not just a matter of clarifying what happens with couples having the same exact "old". We should probably also explain that the first couple is given higher priority when there are couples with overlapping "old"s. I'm not even sure this behaviour is actually mandated by the API, or if it's just an artefact of the current implementation. |
Yes, that's what I meant.
Well, obviously it was never documented, but looking at the code I presume it was intentional: the generic algorithm uses a lookup trie for prioritized key/value pairs, and keys are added with decreasing priority, here ( And I can think of situations where you need to rely on this priority, that's why I said it's a pity. |
What version of Go are you using (
go version
)?go1.10.1
Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?amd64
darwin
What did you do?
https://play.golang.org/p/oB5SCKv4LUx
What did you expect to see?
I expected Replace to return
bbbxxx
.The documentation says:
I expect the aaa->xxx replacement to happen first so the second replacement bbbaaa->yyy shouldn't happen.
What did you see instead?
yyy
The text was updated successfully, but these errors were encountered: