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

Prevent many small highlighted blocks in diff. #336

Merged
merged 1 commit into from Feb 20, 2019

Conversation

@harrysarson
Copy link
Collaborator

commented Feb 10, 2019

By comparing the number of edges of highlighted blocks to the number
of non-highlighted characters (highlighting is the result of
diffing), messy diffs with lots of small highlighted blocks are
disabled.

Diffs will only be displayed if there are more plain characters than there
are edges highlighted blocks in both the actual and expected strings.

Fixes #263.

@harrysarson

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 10, 2019

I have not added a test as there seemed to be no where to put it 😢. Here is a test I used to verify this change locally:

module Example exposing (suite)

import Expect
import Test exposing (..)


suite : Test
suite =
    describe "union type diff"
        [ test "1" <|
            \() ->
                Err { description = "An error", context = "Explanation of what went so wrong" }
                    |> Expect.equal (Ok "Success.")
        , test "2" <|
            \() ->

                Err { context = "Success" }

                    |> Expect.equal (Ok "Success.")

        , test "3" <|
            \() ->
                Err { context = "OhK3 S-u5c6c4e2s2s  4" }
                    |> Expect.equal (Ok "Success.")
        ]

Before:

image

After:

image

Prevent many small highlighted blocks in diff.
By comparing the number of edges of highlighted blocks to the number
of non-highlighted characters (highlighting is the result of
diffing),  messy diffs with lots of small highlighted blocks are
disabled.

Diffs will only be displayed if there are more plain characters than there
are edges highlighted blocks in both the actual and expected strings.

Fixes #263.

@harrysarson harrysarson force-pushed the harrysarson:better-highlight branch from 338cb4b to 812fecb Feb 10, 2019

@jackfranklin

This comment has been minimized.

Copy link
Collaborator

commented Feb 19, 2019

@harrysarson this is awesome and a great change. I think we should figure out some place to put that test you were using as your test case, as I think it'll probably be a useful reference. I'm also not quite sure where in the repo it could go though.

@harrysarson

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2019

I don't think the test runner has any (direct) tests of its elm code at all.

@harrysarson

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 19, 2019

I have found a place to put elm unit tests but I will make a new PR for that as my idea will require some other changes. I would say to merge this as is an I will add the tests soon. :)

(Unless you have any feedback on my code that is).

harrysarson added a commit to harrysarson/node-test-runner that referenced this pull request Feb 20, 2019

Separate elm src and add unit tests
In order to add a test for
rtfeldman#336, this commit
moves all elm source files from `//src` to `//elm/src`.

A unit test has also been added at `//elm/tests/`.
The `//elm/elm.json` file is used when generating the source and for
running unit tests.
There is still `//elm.json` but this is now used only by
`//tests/ci.json`.
@jackfranklin

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2019

It looks great to me, however I'm not familiar with this part of the codebase, so I think I'd like to let @rtfeldman have a look before merging :)

@rtfeldman

This comment has been minimized.

Copy link
Owner

commented Feb 20, 2019

I dig it! This has been bothering me since I added it. 😄

Thanks @harrysarson!

@rtfeldman rtfeldman merged commit 877d818 into rtfeldman:master Feb 20, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@harrysarson harrysarson deleted the harrysarson:better-highlight branch Feb 20, 2019

harrysarson added a commit to harrysarson/node-test-runner that referenced this pull request Feb 20, 2019

Separate elm src and add unit tests
In order to add a test for
rtfeldman#336, this commit
moves all elm source files from `//src` to `//elm/src`.

A unit test has also been added at `//elm/tests/`.
The `//elm/elm.json` file is used when generating the source and for
running unit tests.
There is still `//elm.json` but this is now used only by
`//tests/ci.json`.

harrysarson added a commit to harrysarson/node-test-runner that referenced this pull request Feb 25, 2019

Separate elm src and add unit tests
In order to add a test for
rtfeldman#336, this commit
moves all elm source files from `//src` to `//elm/src`.

A unit test has also been added at `//elm/tests/`.
The `//elm/elm.json` file is used when generating the source and for
running unit tests.
There is still `//elm.json` but this is now used only by
`//tests/ci.json`.

harrysarson added a commit to harrysarson/node-test-runner that referenced this pull request Mar 7, 2019

Separate elm src and add unit tests
In order to add a test for
rtfeldman#336, this commit
moves all elm source files from `//src` to `//elm/src`.

A unit test has also been added at `//elm/tests/`.
The `//elm/elm.json` file is used when generating the source and for
running unit tests.
There is still `//elm.json` but this is now used only by
`//tests/ci.json`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.