Skip to content

Commit

Permalink
Code review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
manfred-brands committed Apr 18, 2024
1 parent b88aa23 commit 8e440fa
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public void VerifyAreEqualFixWhenWithMessageAndArgsArray()
public void Test(object actual, object expected)
{
object[] args = { expected, actual };
ClassicAssert.AreEqual(expected, actual, ""Expected: {0} Got: {1}"", args);
ClassicAssert.AreEqual(expected, actual, ""Expected: {0} Got: {1}"", args);
}");
var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
[TestCase(3.0, 3)]
Expand All @@ -279,7 +279,7 @@ public void VerifyAreEqualFixWhenWithMessageVariable()
[TestCase(3.0, 3)]
public void Test(object actual, object expected)
{
ClassicAssert.AreEqual(expected, actual, GetLocalizedFormatSpecification(), expected, actual);
ClassicAssert.AreEqual(expected, actual, GetLocalizedFormatSpecification(), expected, actual);
}
private static string GetLocalizedFormatSpecification() => ""Expected: {0} Got: {1}"";
");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
bool argsIsArray = !string.IsNullOrEmpty(diagnostic.Properties[AnalyzerPropertyKeys.ArgsIsArray]);

argumentNamesToArguments.TryGetValue(NUnitFrameworkConstants.NameOfMessageParameter, out ArgumentSyntax? messageArgument);
if (CodeFixHelper.GetInterpolatedMessageArgumentOrDefault(false, argsIsArray, messageArgument, args) is ArgumentSyntax interpolatedMessageArgument)
if (CodeFixHelper.GetInterpolatedMessageArgumentOrDefault(messageArgument, args, unconditional: false, argsIsArray) is ArgumentSyntax interpolatedMessageArgument)
newArguments.Add(interpolatedMessageArgument);

var newArgumentsList = invocationNode.ArgumentList.WithArguments(newArguments);
Expand Down
26 changes: 9 additions & 17 deletions src/nunit.analyzers/Helpers/CodeFixHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ internal static class CodeFixHelper
/// <summary>
/// This is assumed to be arguments for an 'Assert.That(actual, constraint, "...: {0} - {1}", param0, param1)`
/// which needs converting into 'Assert.That(actual, constraint, $"...: {param0} - {param1}").
/// Unless we cannot, in which case we create 'string.Format(specification, args)'.
/// Unless we cannot, in which case we create '() => string.Format(specification, args)'.
/// </summary>
/// <param name="unconditional">If the message is not conditional on the test outcome.</param>
/// <param name="argsIsArray">The params args is passed as an array iso individual parameters.</param>
/// <param name="messageArgument">The argument that corresponds to the composite format string.</param>
/// <param name="args">The list of arguments that correspond to format items.</param>
public static ArgumentSyntax? GetInterpolatedMessageArgumentOrDefault(bool unconditional, bool argsIsArray, ArgumentSyntax? messageArgument, List<ArgumentSyntax> args)
/// <param name="unconditional">If the message is not conditional on the test outcome.</param>
/// <param name="argsIsArray">The params args is passed as an array instead of individual parameters.</param>
public static ArgumentSyntax? GetInterpolatedMessageArgumentOrDefault(ArgumentSyntax? messageArgument, List<ArgumentSyntax> args, bool unconditional, bool argsIsArray)
{
if (messageArgument is null)
return null;
Expand Down Expand Up @@ -86,15 +86,7 @@ internal static class CodeFixHelper
.WithArgumentList(SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(
args.Select(x => x.WithNameColon(null)).Prepend(messageArgument))));

if (unconditional)
{
return SyntaxFactory.Argument(stringFormat);
}
else
{
var stringFormatLambda = SyntaxFactory.ParenthesizedLambdaExpression(stringFormat);
return SyntaxFactory.Argument(stringFormatLambda);
}
return SyntaxFactory.Argument(unconditional ? stringFormat : SyntaxFactory.ParenthesizedLambdaExpression(stringFormat));
}

var formatSpecification = literalExpression.Token.ValueText;
Expand All @@ -114,17 +106,17 @@ internal static class CodeFixHelper
/// </summary>
/// <param name="arguments">The arguments passed to the 'Assert' method. </param>
/// <param name="minimumNumberOfArguments">The argument needed for the actual method, any more are assumed messages.</param>
public static void UpdateStringFormatToFormattableString(bool argsIsArray, List<ArgumentSyntax> arguments, int minimumNumberOfArguments)
public static void UpdateStringFormatToFormattableString(List<ArgumentSyntax> arguments, int minimumNumberOfArguments, bool argsIsArray)
{
// If only 1 extra argument is passed, it must be a non-formattable message.
if (arguments.Count <= minimumNumberOfArguments + 1)
return;

ArgumentSyntax? message = GetInterpolatedMessageArgumentOrDefault(
minimumNumberOfArguments == 0,
argsIsArray,
arguments[minimumNumberOfArguments],
arguments.Skip(minimumNumberOfArguments + 1).ToList());
arguments.Skip(minimumNumberOfArguments + 1).ToList(),
unconditional: minimumNumberOfArguments == 0,
argsIsArray);

var nextArgument = minimumNumberOfArguments;
if (message is not null)
Expand Down
2 changes: 1 addition & 1 deletion src/nunit.analyzers/Helpers/DiagnosticsHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ internal static class DiagnosticsHelper
{
public static bool LastArgumentIsNonParamsArray(ImmutableArray<IArgumentOperation> arguments)
{
// Find out of the 'params' argument is an existing array and not one created from a params creation.
// Find out if the 'params' argument is an existing array and not one created from a params creation.
return arguments[arguments.Length - 1].ArgumentKind != ArgumentKind.ParamArray;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private static void AnalyzeNUnit4AssertInvocation(OperationAnalysisContext conte
// The argument after the message is explicitly specified
// Most likely the user thought it was using a format specification with a parameter.
// Or it copied code from some NUnit 3.x source into an NUNit 4 project.
ReportDiagnostic(context, assertOperation, methodName, formatParameterIndex, false);
ReportDiagnostic(context, assertOperation, methodName, formatParameterIndex, argsIsArray: false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
var minimumNumberOfArguments = int.Parse(diagnostic.Properties[AnalyzerPropertyKeys.MinimumNumberOfArguments]!, CultureInfo.InvariantCulture);
bool argsIsArray = !string.IsNullOrEmpty(diagnostic.Properties[AnalyzerPropertyKeys.ArgsIsArray]);

CodeFixHelper.UpdateStringFormatToFormattableString(argsIsArray, arguments, minimumNumberOfArguments);
CodeFixHelper.UpdateStringFormatToFormattableString(arguments, minimumNumberOfArguments, argsIsArray);

var newArgumentsList = invocationNode.ArgumentList.WithArguments(arguments);
var newInvocationNode = invocationNode.WithArgumentList(newArgumentsList);
Expand Down

0 comments on commit 8e440fa

Please sign in to comment.