ChrisPenner / rasa

Extremely modular text editor built in Haskell
GNU General Public License v3.0
616 stars 42 forks source link

Fix combineSpans #61

Open jmatsushita opened 6 years ago

jmatsushita commented 6 years ago
> combineSpans [Span (Range (Coord 0 0) (Coord 0 2)) "a", Span (Range (Coord 0 1) (Coord 0 1)) "b"]
[((Coord (row 0) (col 0)),"a"),((Coord (row 0) (col 1)),"ab"),((Coord (row 0) (col 1)),"a"),((Coord (row 0) (col 2)),"")]

Representing this in ASCII

“aaa”
“ b ”
—— currently combines to
  b
  a
“aa.“

Where vertical stacking is mappend and . is mempty

Where we would expect:

“aaa”
“ b ”
—— combines to
  b
“aaa“
jmatsushita commented 5 years ago

Hey @ChrisPenner I'm giving this a shot. Can I check with you that the tests below reflect the semantics we're after?

  describe "combineSpans" $ do

    -- semantics are that in `Range start end`
    --   - start is inclusive.
    --   - end is exclusive.

    it "should ignore spans with 0 size ranges" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 0)) "a"
        , Span (Range (Coord 0 0) (Coord 0 0)) "b"
        ]
        `shouldBe`
        [
        ]

        -- expected: []
        --  but got: [((Coord 0 0),"a"),((Coord 0 0),""),((Coord 0 0),"b"),((Coord 0 0),"")]

    it "should combine spans containing mempty" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 2)) ""
        , Span (Range (Coord 0 0) (Coord 0 2)) ""
        ]
        `shouldBe`
        [ (Coord 0 0, "")
        ]

        -- expected: [((Coord 0 0),"")]
        --  but got: [((Coord 0 0),""),((Coord 0 0),""),((Coord 0 2),""),((Coord 0 2),"")]

    it "should combine consecutive spans" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 1)) "a"
        , Span (Range (Coord 0 1) (Coord 0 2)) "b"
        ]
        `shouldBe`
        [ (Coord 0 0,"a")
        , (Coord 0 1,"b")
        , (Coord 0 2,"")
        ]

        -- expected: [((Coord 0 0),"a"),((Coord 0 1),"b"),((Coord 0 2),"")]
        --  but got: [((Coord 0 0),"a"),((Coord 0 1),""),((Coord 0 1),"b"),((Coord 0 2),"")]

    it "should combine full spans" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 2)) "a"
        , Span (Range (Coord 0 0) (Coord 0 2)) "b"
        ]
        `shouldBe`
        [ (Coord 0 0,"ab")
        , (Coord 0 2,"")
        ]

        -- expected: [((Coord 0 0),"ab"),((Coord 0 2),"")]
        --  but got: [((Coord 0 0),"a"),((Coord 0 0),"ab"),((Coord 0 2),"b"),((Coord 0 2),"")]

    it "should combine three full spans" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 2)) "a"
        , Span (Range (Coord 0 0) (Coord 0 2)) "b"
        , Span (Range (Coord 0 0) (Coord 0 2)) "c"
        ]
        `shouldBe`
        [ (Coord 0 0,"abc")
        , (Coord 0 2,"")
        ]

        -- expected: [((Coord 0 0),"abc"),((Coord 0 2),"")]
        --  but got: [((Coord 0 0),"a"),((Coord 0 0),"ab"),((Coord 0 0),"bac"),((Coord 0 2),"cb"),((Coord 0 2),"c"),((Coord 0 2),"")]

    it "should combine overlapping spans" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 3)) "a"
        , Span (Range (Coord 0 1) (Coord 0 2)) "b"
        ]
        `shouldBe`
        [ (Coord 0 0,"a")
        , (Coord 0 1,"ab")
        , (Coord 0 2,"a")
        , (Coord 0 3,"")
        ]

        -- PASSES !!!
ChrisPenner commented 5 years ago

@jmatsushita This is very close; the current behaviour is that spans are 'inclusive' in their end position, i.e. Span (Range (Coord 0 0) (Coord 0 0)) "a" DOES contain the character at 0 0, whereas in your tests it appears to be exclusive in the end-point of the range. So for example this test case:

it "should ignore spans with 0 size ranges" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 0)) "a"
        , Span (Range (Coord 0 0) (Coord 0 0)) "b"
        ]
        `shouldBe`
        [
        ]

According to my understanding should instead be:

it "should ignore spans with 0 size ranges" $ do
      combineSpans
        [ Span (Range (Coord 0 0) (Coord 0 0)) "a"
        , Span (Range (Coord 0 0) (Coord 0 0)) "b"
        ]
        `shouldBe`
        [ (Coord 0 0,"ab")
        , (Coord 0 1,"")
        ]

We could of course change the semantics of the Span itself, but I believe the existing work which uses them treats them as inclusive.