-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Support for nested/pointer/byref types for Type.ToTypeString() #3468
Support for nested/pointer/byref types for Type.ToTypeString() #3468
Conversation
Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, jk about the Llama, but am curious about the output from 6.1 to now. 🙂
} | ||
|
||
return type.ToString(); | ||
return $"{type.Namespace}.{displayName}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the output here significantly different or just better formatted here? i.e. if someone with 6.1 upgrades, will the result be different in the cases where things worked?
(Just want to check if we should mark this as a breaking change even though the API surface itself didn't change.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output should be the same for types that were properly supported already 😊
In that line I'm explicitly adding the namespace just because with this change we're now using the name of each type directly instead of the fullname (so that we can manually traverse each type in case of nested types), so we need to add the namespace at the start ourselves to match the previous output with the fully qualified name.
As for changes between this PR and the previous version, here's some examples.
I'm only displaying cases with actual differences, such as nested types or pointer/ref types:
typeof(Animal.Cat)
// 6.1: Namespace.Animal+Cat (uses + instead of .)
// 7.0: Namespace.Animal.Cat
typeof(Animal.Cat<int>)
// 6.1: Namespace.Animal+Cat<int> (same)
// 7.0: Namespace.Animal.Cat<int>
typeof(Animal.Cat<int>.Bar)
// 6.1: Namespace.Animal+Cat<int> (the nested .Bar type got lost)
// 7.0: Namespace.Animal.Cat<int>.Bar
typeof(Animal.Llama<string, int[]>.Foo)
// 6.1: Namespace.Animal+Llama<string, int[]> (same)
// 7.0: Namespace.Animal.Llama<string, int[]>.Foo
typeof(Animal.Llama<string, int[]>.Foo<byte>)
// 6.1: Namespace.Animal+Llama<string, int[], byte> (nested type missing, wrong type arguments)
// 7.0: Namespace.Animal.Llama<string, int[]>.Foo<byte>
typeof(void)
// 6.1: System.Void
// 7.0: void
typeof(int**)
// 6.1: System.Int32**
// 7.0: int**
typeof(string).MakeByRefType()
// 6.1: System.String&
// 7.0: string&
In general, some cases just look nicer (eg. .
instead of +
for a simple nested type, or void
instead of System.Void
), while others are now actually correct, whereas currently they are either missing bits (eg. nested types getting lost in the process) and/or displayed with wrong type arguments (they're basically all combined together).
[DataRow("UnitTests.Extensions.Test_TypeExtensions.Animal.Rabbit<int>.Foo<string>", typeof(Animal.Rabbit<int>.Foo<string>))] | ||
[DataRow("UnitTests.Extensions.Test_TypeExtensions.Animal.Rabbit<int>.Foo<int[]>", typeof(Animal.Rabbit<int>.Foo<int[]>))] | ||
[DataRow("UnitTests.Extensions.Test_TypeExtensions.Animal.Rabbit<string[]>.Foo<object>", typeof(Animal.Rabbit<string[]>.Foo<object>))] | ||
[DataRow("UnitTests.Extensions.Test_TypeExtensions.Animal.Rabbit<(string, int)?>.Foo<(int, int?)>", typeof(Animal.Rabbit<(string, int)?>.Foo<(int, int?)>))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No 🦙? 😒
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too late! e74eb65 🙈
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Hello @RosarioPulella! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Follow up for #3131
The
Type.ToTypeString()
introduced with theGuard
API lacks support for nested types, pointer types and byref types.PR Type
What kind of change does this PR introduce?
What is the current behavior?
The extension doesn't work properly when used with nested, pointer or byref types.
It will either crash or just return the wrong formatted type (eg. the declaring type instead of the nested type).
What is the new behavior?
The extension now works correctly with all sorts of nested, pointer and byref types 🎉
PR Checklist
Please check if your PR fulfills the following requirements:
Pull Request has been submitted to the documentation repository instructions.Sample in sample app has been added / updated (for bug fixes / features)Icon has been created (if new sample) following the Thumbnail Style Guide and templates