Skip to content
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

Fix combineSpans #61

Open
jmatsushita opened this issue Nov 7, 2018 · 2 comments
Open

Fix combineSpans #61

jmatsushita opened this issue Nov 7, 2018 · 2 comments

Comments

@jmatsushita
Copy link
Collaborator

> 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
Copy link
Collaborator Author

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
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants