WIP: Omit plus and minus in string diffs, except in text mode

[papandreou]

Inspiration: https://github.com/so-fancy/diff-so-fancy

[papandreou]

Here it would be nice of magicPen.alt allowed omitting fallback: function () {}.

[sunesimonsen]

What would happen in the unexpected-pdf ;-)

[papandreou]

It just wouldn't do anything in that mode. It'd just assume that you wanted to fall back to NOOP :)

[sunesimonsen]

Let's make the fallback default to a noop then.

[papandreou]

Great, on it.

[papandreou]

Fixed in magicpen 5.6.0, updated to it on the branch.

[sunesimonsen]

I'm okay with putting all the formatting in a style. But I would like the actual diffing to happen somewhere else. Think: spilling computation from presentation.

I'll get back to you with more feedback, probably tonight.

[sunesimonsen]

I like the main idea of loosing the + and - signs, but I don't think we should put things in styles that is not about styling. If we can create a string diff data structure and hand that of to a style I would prefer that. I would think it is possible to just have the + and - in an alt style. It is okay to create more styles that is capable of formating a added/removed line.

[papandreou]

I like the main idea of loosing the + and - signs

Great, that's the main thing at this point :)

I don't think we should put things in styles that is not about styling. If we can create a string diff data structure and hand that of to a style I would prefer that.

I thought that we already sort of had that in the shape of require('diff').diffLines? You're right that we have added some postprocessing that isn't all about styling per se, but I think we're pretty close to what you want already?

[sunesimonsen]

Okay I'll take a second look at what's going on. I still think it is possible to achieve this without using prependLinesWith.

[papandreou]

@sunesimonsen Thanks, ironically enough I'm beginning to think it's a bit dirty to manipulate the contents of a pen.

Feel free to push to the branch if you get anywhere ofc :)

[sunesimonsen]

I agree, I would probably have made it immutable today 😁

[sunesimonsen]

This is the part I don't really like, the style does the actual diff computation instead of being handed the diff. I know that the API from the outside would be convenient by letting the style taking care of it, but I just think it is misplaced. I would prefer:

var diff = utils.stringDiff(actual, expected)
pen.appendDiff(diff)

It is mainly about separation of concerns.

[sunesimonsen]

This would of cause mean that we would need to change the output of the utils.stringDiff.

[sunesimonsen]

I think the the diff formatting codes looks a bit weird, maybe it would also help to just start over. Make a utils.stringDiff that produces a data structure that contains lines of tokens. Then hand that output to a magicpen style that would format it the way we want. Then it should also be possible to get rid of `prependLinesWith'.

[sunesimonsen]

Shit I don't know, maybe we should just allow magicpen to do these crazy things, magicpen-prism is in the same space. It just feels a bit wrong, but what ever :-) Could I take a stab at fixing up the formatting code?

I realize that I'm having a conversation with myself and convincing myself of going in new directions :-)

[sunesimonsen]

Shit this is much more complicated than I remembered :-)

[sunesimonsen]

I think we can simplify things if we only used diffAddedLine, diffRemovedLine with one line and not multiple lines. Then we would be able to move the -/+ signs to these methods:

    expect.addStyle('diffAddedLine', function (line) {
        this.alt({ text: '+' });
        this.green(line);
    });

    expect.addStyle('diffRemovedLine', function (line) {
        this.alt({ text: '-' });
        this.red(line);
    });

    expect.addStyle('diffUnchangedLine', function (line){
        this.alt({ text: ' ' });
        this.text(line)
    });

[papandreou]

That's also what I arrived at. I attempted it once, but got stuck when I ran into the "internal" diff that the handling of replaced chunks does.

[sunesimonsen]

I'm not going to block this improvement with a cleanup of the stringDiff - it was fucked before, that situation has not changed just because the turd moved.

👍

[papandreou]

I'm not at all satisfied with the state this is in myself, but you're right that the cleanup might be better handled as a separate task. Let's discuss that here: #272

Merging.