[bug] Code fix for NUnit2049 places the comma at a wrong place and messes up indentation

[Bartleby2718]

// before the code fix
CollectionAssert.AreEquivalent(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." },
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." }
);

is transformed into

// after the current code fix
Assert.That(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." }
, Is.EquivalentTo(new[] { "This is such a long sentence that I don't want the other parameter on the same line." }));

There are two problems with this:

1. To be consistent with the original formatting, the comma should be trailing, not leading.
2. The second argument should have the same indentation as the first argument.

// after the proposed code fix
Assert.That(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." },
	Is.EquivalentTo(new[] { "This is such a long sentence that I don't want the other parameter on the same line." }));

Similarly,

// before the code fix
CollectionAssert.AreEquivalent(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." },
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." });

is transformed into

// after the current code fix
Assert.That(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." }, Is.EquivalentTo(new[] { "This is such a long sentence that I don't want the other parameter on the same line." }));

but I believe the right formatting should be

// after the proposed code fix
Assert.That(
	new[] { "This is such a long sentence that I don't want the other parameter on the same line." },
	Is.EquivalentTo(new[] { "This is such a long sentence that I don't want the other parameter on the same line." }));

I believe fixing the former will also fix the latter.

[manfred-brands]

@Bartleby2718 I agree with your right formatting. However it isn't that simple as to which syntax node the compiler assigns white space.

CollectionAssert.AreEquivalent(
    collection1,
    collection2
);

Syntax	ToFullString
invocationNode.ArgumentList	"(\r\n collection1,\r\n collection2\r\n )"
invocationNode.ArgumentList.OpenParenToken	"(\r\n"
invocationNode.ArgumentList.Arguments[0]	" collection1"
invocationNode.ArgumentList.Arguments[1]	" collection2\r\n"
invocationNode.ArgumentList.Arguments.GetSeparator(0)	",\r\n"

Note that the newline after the comma is associated with the comma, not the arguments. The call var newArgumentsList = invocationNode.ArgumentList.WithArguments(arguments); creates a new argument list with just a comma as separator and the new line is lost.

Yes diving down it is possible to retrieve and maintain the original separator with it trailing newline. I managed to get the above example to work.

However. It gets even trickier if comments are used:

CollectionAssert.AreEquivalent(
    collection1, // expected
    collection2 // actual
);

The // expected is associated with the comma separator, not the collection1 So the CodeFix I made for the initiali problem that moves trivia from argument 1 to 0 and vice versa creates:

Assert.That(
    collection2, // expected
    Is.EqualTo(collection1),AsCollection  // actual
);

Keeping the trailing trivia with the argument results in:

Assert.That(
    collection2 // actual
, // expected
    Is.EqualTo(collection1),AsCollection);

Also not what you would want,

The TrailingTrivia for collection2 parameter does not only contain the // actual comment but also a trailing \r\n. This would require splitting the \r\n from the trivia.

So to make this work for all possible combination to me seems to be a lot of work and still likely to go wrong.

[Bartleby2718]

@manfred-brands Agree that there are many edge cases and it's not easy to cover all of them. However, the current behavior covers none of them, and handling whitespace correctly should be a decent win.