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.

Sign up for GitHub

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

Render array moves with arrows #333

Merged
merged 9 commits into from
Sep 26, 2016
Merged

Render array moves with arrows #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0738cf6
Update array-changes-async to 3.0.0 and array-changes to 2.0.0.
papandreou Sep 11, 2016
4357884
array-like: Render moves with arrows
papandreou Aug 24, 2016
eac213a
Use greedy-interval-packer to avoid reserving too much horizontal spa…
papandreou Sep 12, 2016
38f0c46
Render arrow heads with ▷ instead of >
papandreou Sep 12, 2016
8966ca6
Update expected output in a test where the array-changes fix causes i…
papandreou Sep 14, 2016
accb2f2
Pack using the change indices instead of the measured heights of the …
papandreou Sep 15, 2016
2110adf
Separate the packing from the arrowsAlongsideChangeOutputs style.
papandreou Sep 15, 2016
b9839d8
Make sure that we render a max of 3 arrow lanes.
papandreou Sep 15, 2016
917723b
Avoid trailing whitespace when the first change is a moveTarget item.
papandreou Sep 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions documentation/api/addAssertion.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,11 @@ expect([ 1, 3, 2, 4 ], 'to be sorted');
expected [ 1, 3, 2, 4 ] to be sorted

[
1,
3, // should equal 2
2, // should equal 3
4
1,
┌─▷
│ 3,
└── 2, // should be moved
4
]
```

Expand All @@ -160,10 +161,11 @@ expect([ 1, 3, 2, 4 ], 'to be sorted');
expected [ 1, 3, 2, 4 ] to equal [ 1, 2, 3, 4 ]

[
1,
3, // should equal 2
2, // should equal 3
4
1,
┌─▷
│ 3,
└── 2, // should be moved
4
]
```

Expand All @@ -182,10 +184,11 @@ expected [ 1, 3, 2, 4 ] to be sorted
expected [ 1, 3, 2, 4 ] to equal [ 1, 2, 3, 4 ]

[
1,
3, // should equal 2
2, // should equal 3
4
1,
┌─▷
│ 3,
└── 2, // should be moved
4
]
```

Expand All @@ -203,10 +206,11 @@ expect([ 1, 3, 2, 4 ], 'to be sorted');
expected [ 1, 3, 2, 4 ] to be sorted

[
1,
3, // should equal 2
2, // should equal 3
4
1,
┌─▷
│ 3,
└── 2, // should be moved
4
]
```

Expand All @@ -222,10 +226,11 @@ expect([ 1, 3, 2, 4 ], 'to be sorted');

```output
[
1,
3, // should equal 2
2, // should equal 3
4
1,
┌─▷
│ 3,
└── 2, // should be moved
4
]
```

Expand Down
8 changes: 5 additions & 3 deletions documentation/assertions/array-like/when-sorted-by.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ expected [ 2, 1, 3 ]
when sorted by function (a, b) { return a - b; } to equal [ 3, 2, 1 ]

[
1, // should equal 3
2,
3 // should equal 1
┌───▷
│ ┌─▷
│ │ 1,
│ └── 2, // should be moved
└──── 3 // should be moved
]
```
14 changes: 5 additions & 9 deletions documentation/assertions/array-like/when-sorted.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,10 @@ expect(['c', 'a', 'b'], 'when sorted', 'to equal', ['c', 'b', 'a']);
expected [ 'c', 'a', 'b' ] when sorted to equal [ 'c', 'b', 'a' ]

[
'a', // should equal 'c'
//
// -a
// +c
'b',
'c' // should equal 'a'
//
// -c
// +a
┌───▷
│ ┌─▷
│ │ 'a',
│ └── 'b', // should be moved
└──── 'c' // should be moved
]
```
107 changes: 58 additions & 49 deletions lib/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1044,59 +1044,68 @@ module.exports = function (expect) {
if (subjectType.indent) {
output.indentLines();
}
changes.forEach(function (diffItem, index) {
var packing = utils.packArrows(changes); // NOTE: Will have side effects in changes if the packing results in too many arrow lanes
output.arrowsAlongsideChangeOutputs(packing, changes.map(function (diffItem, index) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really has nothing to do with the PR, but I think we have crossed the line where we should split up the assertions in separates files (Not in this PR - but after). After we split the assertions into separate files, I think something like this would be much more readable if it was split into some more helper methods.

var delimiterOutput = subjectType.delimiter(output.clone(), index, indexOfLastNonInsert + 1);
output.nl(index > 0 ? 1 : 0).i().block(function () {
var type = diffItem.type;
if (type === 'insert') {
this.annotationBlock(function () {
var index = typeof diffItem.actualIndex !== 'undefined' ? diffItem.actualIndex : diffItem.expectedIndex;
if (expect.findTypeOf(diffItem.value).is('function')) {
this.error('missing: ')
.property(index, output.clone().block(function () {
this.omitSubject = undefined;
var promise = keyPromises[diffItem.expectedIndex];
if (promise.isRejected()) {
this.appendErrorMessage(promise.reason());
} else {
this.appendInspected(diffItem.value);
}
}), true);
} else {
this.error('missing ').property(index, inspect(diffItem.value), true);
}
});
} else {
this.property(diffItem.actualIndex, output.clone().block(function () {
if (type === 'remove') {
this.append(inspect(diffItem.value).amend(delimiterOutput.sp()).error('// should be removed'));
} else if (type === 'equal') {
this.append(inspect(diffItem.value).amend(delimiterOutput));
} else {
var toSatisfyResult = toSatisfyMatrix[diffItem.actualIndex][diffItem.expectedIndex];
var valueDiff = toSatisfyResult && toSatisfyResult !== true && toSatisfyResult.getDiff({ output: output.clone() });
if (valueDiff && valueDiff.inline) {
this.append(valueDiff.amend(delimiterOutput));
var type = diffItem.type;
if (type === 'moveTarget') {
return output.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like there should be a magicpen construct to map over an array of items and produce output for each item. I know you are just cloning the output, but it feels a bit unsafe that you might append to the outside output by accident. This is just thoughts on how we might refactor later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

} else {
return output.clone().block(function () {
if (type === 'moveSource') {
this.property(diffItem.actualIndex, inspect(diffItem.value), true)
.amend(delimiterOutput.sp()).error('// should be moved');
} else if (type === 'insert') {
this.annotationBlock(function () {
var index = typeof diffItem.actualIndex !== 'undefined' ? diffItem.actualIndex : diffItem.expectedIndex;
if (expect.findTypeOf(diffItem.value).is('function')) {
this.error('missing: ')
.property(index, output.clone().block(function () {
this.omitSubject = undefined;
var promise = keyPromises[diffItem.expectedIndex];
if (promise.isRejected()) {
this.appendErrorMessage(promise.reason());
} else {
this.appendInspected(diffItem.value);
}
}), true);
} else {
this.error('missing ').property(index, inspect(diffItem.value), true);
}
});
} else {
this.property(diffItem.actualIndex, output.clone().block(function () {
if (type === 'remove') {
this.append(inspect(diffItem.value).amend(delimiterOutput.sp()).error('// should be removed'));
} else if (type === 'equal') {
this.append(inspect(diffItem.value).amend(delimiterOutput));
} else {
this.append(inspect(diffItem.value).amend(delimiterOutput)).sp().annotationBlock(function () {
this.omitSubject = diffItem.value;
var label = toSatisfyResult.getLabel();
if (label) {
this.error(label).sp()
.block(inspect(diffItem.expected));
if (valueDiff) {
this.nl(2).append(valueDiff);
var toSatisfyResult = toSatisfyMatrix[diffItem.actualIndex][diffItem.expectedIndex];
var valueDiff = toSatisfyResult && toSatisfyResult !== true && toSatisfyResult.getDiff({ output: output.clone() });
if (valueDiff && valueDiff.inline) {
this.append(valueDiff.amend(delimiterOutput));
} else {
this.append(inspect(diffItem.value).amend(delimiterOutput)).sp().annotationBlock(function () {
this.omitSubject = diffItem.value;
var label = toSatisfyResult.getLabel();
if (label) {
this.error(label).sp()
.block(inspect(diffItem.expected));
if (valueDiff) {
this.nl(2).append(valueDiff);
}
} else {
this.appendErrorMessage(toSatisfyResult);
}
} else {
this.appendErrorMessage(toSatisfyResult);
}
});
});
}
}
}
}), true);
}
});
});
}), true);
}
});
}
}));

if (subjectType.indent) {
output.outdentLines();
}
Expand Down
114 changes: 114 additions & 0 deletions lib/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,4 +347,118 @@ module.exports = function (expect) {
}
}, this);
});

expect.addStyle('arrow', function (options) {
options = options || {};
var styles = options.styles || [];
var i;
this.nl(options.top || 0).sp(options.left || 0).text('┌', styles);
for (i = 1 ; i < options.width ; i += 1) {
this.text(i === options.width - 1 && options.direction === 'up' ? '▷' : '─', styles);
}
this.nl();
for (i = 1 ; i < options.height - 1 ; i += 1) {
this.sp(options.left || 0).text('│', styles).nl();
}
this.sp(options.left || 0).text('└', styles);
for (i = 1 ; i < options.width ; i += 1) {
this.text(i === options.width - 1 && options.direction === 'down' ? '▷' : '─', styles);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make a magicpen-arrow plugin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can make it a bit more generic, and if unexpected-htmllike/-react cannot reach this feature through the array-like type, I agree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it might also mean implementing a lot of features that we wont use.


var flattenBlocksInLines = require('magicpen/lib/flattenBlocksInLines');
expect.addStyle('merge', function (pens) {
var flattenedPens = pens.map(function (pen) {
return flattenBlocksInLines(pen.output);
}).reverse();
var maxHeight = flattenedPens.reduce(function (maxHeight, pen) {
return Math.max(maxHeight, pen.length);
}, 0);
var blockNumbers = new Array(flattenedPens.length);
var blockOffsets = new Array(flattenedPens.length);
// As long as there's at least one pen with a line left:
for (var lineNumber = 0 ; lineNumber < maxHeight ; lineNumber += 1) {
if (lineNumber > 0) {
this.nl();
}
var i;
for (i = 0 ; i < blockNumbers.length ; i += 1) {
blockNumbers[i] = 0;
blockOffsets[i] = 0;
}
var contentLeft;
do {
contentLeft = false;
var hasOutputChar = false;
for (i = 0 ; i < flattenedPens.length ; i += 1) {
var currentLine = flattenedPens[i][lineNumber];
if (currentLine) {
while (currentLine[blockNumbers[i]] && blockOffsets[i] >= currentLine[blockNumbers[i]].args.content.length) {
blockNumbers[i] += 1;
blockOffsets[i] = 0;
}
var currentBlock = currentLine[blockNumbers[i]];
if (currentBlock) {
contentLeft = true;
if (!hasOutputChar) {
var ch = currentBlock.args.content.charAt(blockOffsets[i]);
if (ch !== ' ') {
this.text(ch, currentBlock.args.styles);
hasOutputChar = true;
}
}
blockOffsets[i] += 1;
}
}
}
if (!hasOutputChar && contentLeft) {
this.sp();
}
} while (contentLeft);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked about, this style should move to magicpen core when it has proven it's stability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


expect.addStyle('arrowsAlongsideChangeOutputs', function (packing, changeOutputs) {
if (packing) {
var topByChangeNumber = {};
var top = 0;
changeOutputs.forEach(function (changeOutput, index) {
topByChangeNumber[index] = top;
top += changeOutput.size().height;
});
var that = this;

var arrows = [];
packing.forEach(function (columnSet, i, packing) {
columnSet.forEach(function (entry) {
arrows.push(that.clone().arrow({
left: i * 2,
top: topByChangeNumber[entry.start],
width: 1 + (packing.length - i) * 2,
height: topByChangeNumber[entry.end] - topByChangeNumber[entry.start] + 1,
direction: entry.direction
}));
});
});

if (arrows.length === 1) {
this.block(arrows[0]);
} else if (arrows.length > 1) {
this.block(function () {
this.merge(arrows);
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If would be better if this was just:

this.block(function () {
  this.merge(arrows);
});

Magicpen should be smart enough to avoid the extra block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and merge should be smart enough to handle one pen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way merge is implemented, it takes an array of pens and produces a fresh pen with the merged output. It does handle the single pen case, but I'd like to avoid creating a copy here. My initial attempt at implementing the merging was to merge one or more other pens into the current pen, but that turned out to be harder to implement and not as understandable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial attempt at implementing the merging was to merge one or more other pens into the current pen, but that turned out to be harder to implement and not as understandable.

Ohh I though it worked that way. That is probably how I want it to work in magicpen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I agree that would be the most useful API-wise.

} else {
this.i();
}

this.block(function () {
changeOutputs.forEach(function (changeOutput, index) {
this.nl(index > 0 ? 1 : 0);
if (!changeOutput.isEmpty()) {
this.sp(packing ? 1 : 0).append(changeOutput);
}
}, this);
});
});
};
Loading