Skip to content

Add support to tuples in inline values (#4771)#4772

Merged
stevenaw merged 5 commits into
nunit:mainfrom
MaxKot:main
Sep 30, 2024
Merged

Add support to tuples in inline values (#4771)#4772
stevenaw merged 5 commits into
nunit:mainfrom
MaxKot:main

Conversation

@MaxKot
Copy link
Copy Markdown
Contributor

@MaxKot MaxKot commented Jul 30, 2024

Fixes #4771

Please note that merging this also requires an update to NUnit1031 diagnostic in the analyzers repository.

Comment on lines +171 to +177
if (genericType.Assembly == typeof(ValueTuple).Assembly &&
genericType.Namespace == typeof(ValueTuple).Namespace &&
genericType.Name.StartsWith("ValueTuple`", StringComparison.Ordinal))
{
typeArgs = type.GetGenericArguments();
return true;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There should probably be tests of tuples with more than seven elements, since the C# type (T1, T2, T3, T4, T5, T6, T7, T8) maps to the type ValueTuple<T1, T2, T3, T4, T4, T5, T6, T7, ValueTuple<T8>>.

(System.Tuple and System.ValueTuple require the TRest parameter to be a tuple, and this mapping is recursive to allow any number of elements.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed in 14c321b.

@OsirisTerje
Copy link
Copy Markdown
Member

@manfred-brands @stevenaw Can you have a look at this? It's a bit of code here. Are we good with this?

Copy link
Copy Markdown
Member

@manfred-brands manfred-brands left a comment

Choose a reason for hiding this comment

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

@MaxKot Thanks for your contribution.
It looks good.
I have made one possible suggestion to replace the string comparisons.

Comment on lines +201 to +204
Type genericType = type.GetGenericTypeDefinition();
if (genericType.Assembly == typeof(Tuple).Assembly &&
genericType.Namespace == typeof(Tuple).Namespace &&
genericType.Name.StartsWith("Tuple`", StringComparison.Ordinal))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we could have a static field:

private static Type[] TupleTypes = [typeof(Tuple), typeof(Tuple<,>), typeof(Tuple<,,>), ...]

Then use:

typeargs = type.GetGenericArguments();
if (typeargs.Length <= TupleTypes.Length && genericType == TupleTypes[typeargs.Length])

This would eliminate 3 string comparisons (or 6 if counting the ValueTuple).
Probably try it out in your benchmark if it makes any difference.

Copy link
Copy Markdown
Member

@stevenaw stevenaw left a comment

Choose a reason for hiding this comment

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

Thanks @MaxKot this looks great and seems to cover many of the edge cases I can think of. Thanks again for your patience on this review, as well as for your contribution.

@stevenaw stevenaw merged commit 73f3058 into nunit:main Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support tuples in inline data

5 participants