fix Caret arguments ordering #313
Conversation
`line` was filled with `offset`, `col` was filled with `line` and `offset` with `col`. I'm guessing at one point Caret was `Caret(offset, line, column)`, but was changed to `Caret(line, column, offset)` without re-examing argument order. This caused this behavior: ```scala val p = (Parser.caret.with1 <* Parser.anyChar).rep val pattern = """aaa""" val parsed = p.parseAll(pattern).right.get // parsed: cats.data.NonEmptyList[Caret] = NonEmptyList(Caret(0, 0, 0), List(Caret(1, 0, 1), Caret(2, 0, 2))) ``` Expected: ```scala // parsed: cats.data.NonEmptyList[Caret] = NonEmptyList(Caret(0, 0, 0), List(Caret(0, 1, 1), Caret(0, 2, 2))) ```
| @@ -226,7 +226,7 @@ class LocationMapTest extends munit.ScalaCheckSuite { | |||
| val lc = lm.toLineCol(offset) | |||
|
|
|||
| assertEquals(oc, Some(c)) | |||
| assertEquals(lc, oc.map { case Caret(_, r, c) => (r, c) }) | |||
| assertEquals(lc, oc.map { case c => (c.line, c.col) }) | |||
There was a problem hiding this comment.
can we remove the case and the extra space here?
I think you want to run fmt.
There was a problem hiding this comment.
Yeah forgot to run formatting
| @@ -226,7 +226,7 @@ class LocationMapTest extends munit.ScalaCheckSuite { | |||
| val lc = lm.toLineCol(offset) | |||
|
|
|||
| assertEquals(oc, Some(c)) | |||
| assertEquals(lc, oc.map { case Caret(_, r, c) => (r, c) }) | |||
| assertEquals(lc, oc.map { case c => (c.line, c.col) }) | |||
There was a problem hiding this comment.
reminder: can we remove the case now?
Codecov Report
@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 96.28% 96.56% +0.28%
==========================================
Files 9 9
Lines 1049 1049
Branches 94 94
==========================================
+ Hits 1010 1013 +3
+ Misses 39 36 -3
Continue to review full report at Codecov.
|
| @@ -226,7 +226,7 @@ class LocationMapTest extends munit.ScalaCheckSuite { | |||
| val lc = lm.toLineCol(offset) | |||
|
|
|||
There was a problem hiding this comment.
actually while we are at it can we add: assertEquals(c.offset, offset) to make sure we pass through the offset?
|
dang it, I actually feel a bit responsible for this issue ... I should have caught it! Sorry about this! |
|
No need to apologize! |
linewas filled withoffset,colwas filled withlineandoffsetwithcol.I'm guessing at one point Caret was
Caret(offset, line, column), but was changed toCaret(line, column, offset)without re-examing argument order.This caused this behavior:
Expected:
// parsed: cats.data.NonEmptyList[Caret] = NonEmptyList(Caret(0, 0, 0), List(Caret(0, 1, 1), Caret(0, 2, 2)))