Open pithub opened 1 year ago
If this patch fixes:
https://github.com/elm/parser/issues/20
Does that mean it should be preferred to the PR that fixes just that issue:
https://github.com/elm/parser/pull/21
This PR seems more general than that one and fixes a number of issues together.
Hi Rupert, I think you addressed the question to me, but as the author I'm biased, of course. If I wouldn't prefer this PR to #21, then I wouldn't have added it.
This code change fixes the following issues:
and makes the following pull requests superfluous:
As described in issue #53, there's a bug in the
Elm.Kernel.Parser.findSubString
function, that leads to inconsistent internal parser positions where:This code change fixes that bug, so that both offset and row/column are consistently positioned after the token.
There are two reasons why I chose the after token position and not the before token position:
2
20
multiComment
parser when using theNestable
modeThe real bug fix is on this line:
where we either return
-1
to signal the "subString not found" case, or the offset after the subString, which is stored in variabletarget
.So what about all the other changes?
First, I decided to rename the
newOffset
variable:It doesn't contain the new offset anymore (since we return either
-1
ortarget
), and I thought the name would be misleading now. Therefore I changed the name to "index", because it contains the result of theindexOf
function.In the
Parser.Advanced
module, there's a wrapper function for every Kernel function. The comment of thefindSubString
wrapper function has been wrong before this code change (see #37 "Fix comment infindSubString
"), and it was still wrong after the code change, so I changed it to document the fact, that we return the position after the subString:The wrapper functions hide the fact, that they are implemented in JavaScript rather than in Elm, from the rest of the module code. The rest of the code only uses the wrapper functions, just as if they had been implemented in Elm.
The only exception from this rule was a line where the
findSubString
Kernel function has been called directly, and I thought it was appropriate to change this line, too, to be consistent with the rest of the code, when we are modifying thefindSubString
function itself: