Regex.replace does not set the index accurately

[rgrempel]

In this line in Regex.replace:

https://github.com/elm-lang/core/blob/1b28ce5ce8f30ddf549e5f454a772f7b623e09b4/src/Native/Regex.js#L90

there are two problems.

First, the variable i will inevitably be equal to -1 at this point. Thus, saying i - 1 is necessarily equivalent to -2, but obscure.

Second, the arguments object is not really an array, and does not support negative indexes. So, the index property is always set to undefined.

So, it would be better to replace this line with something like:

index: arguments[arguments.length - 2],

[rgrempel]

Here's an illustration of the problem, for use in elm-repl. (I would have written a failing test, but had some trouble getting the tests to run at all). The <internal structure> references below are the undefined results from match.index.

import Regex exposing (..)
findComma = regex(",")
replace All findComma (\match -> toString match.index) "a,b,c"

-- "a<internal structure>b<internal structure>c" : String

[rgrempel]

Here's an illustration of the problem, for use in elm-repl. (I would have written a failing test, but had some trouble getting the tests to run at all). The <internal structure> references below are the undefined results from match.index.

import Regex exposing (..)
findComma = regex(",")
replace All findComma (\match -> toString match.index) "a,b,c"

-- "a<internal structure>b<internal structure>c" : String

[jvoigtlaender]

Yeah, pretty bad. Also reproducible on http://elm-lang.org/try, using this program:

import Graphics.Element
import Regex exposing (..)

findComma = regex(",")

main = Graphics.Element.show <| replace All findComma (\match -> toString match.index) "a,b,c"

Maybe you can revise your original post above to directly include that? Or better yet in a pull request replacing this ticket? 😄

Generally in the line of this comment by Evan.

[jvoigtlaender]

Yeah, pretty bad. Also reproducible on http://elm-lang.org/try, using this program:

import Graphics.Element
import Regex exposing (..)

findComma = regex(",")

main = Graphics.Element.show <| replace All findComma (\match -> toString match.index) "a,b,c"

Maybe you can revise your original post above to directly include that? Or better yet in a pull request replacing this ticket? 😄

Generally in the line of this comment by Evan.

[rgrempel]

To do a proper PR, I would want to write a failing test case that the PR fixes. However, I'm having trouble getting the tests to run at all -- I'll try harder at that when I have time.

[rgrempel]

To do a proper PR, I would want to write a failing test case that the PR fixes. However, I'm having trouble getting the tests to run at all -- I'll try harder at that when I have time.

[jvoigtlaender]

Yeah, it's a pity that the master branch is not in shape to run tests at the moment. Maybe #446 or #447 would help you locally?

[jvoigtlaender]

Yeah, it's a pity that the master branch is not in shape to run tests at the moment. Maybe #446 or #447 would help you locally?

[rgrempel]

Ah, thanks, I will give those a try.

[rgrempel]

Ah, thanks, I will give those a try.

[evancz]

Consolidated regex stuff in #722. Follow along there!

Seems like a PR is possible.

[evancz]

Consolidated regex stuff in #722. Follow along there!

Seems like a PR is possible.