-
Notifications
You must be signed in to change notification settings - Fork 458
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
Make TypeUtil.ToCSharpString produce more C#-like formatting #407
Conversation
20b4d54
to
e3c4ab7
Compare
{ | ||
var name = typeof(GenericHasNested<A2>.Nested).ToCSharpString(); | ||
Assert.AreEqual("GenericHasNested<�TOuter�>.Nested<A2>", name); | ||
Assert.AreEqual("GenericHasNested<A2>.Nested", name); |
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.
Should we add a test that nests generic arguments?
var name = typeof(GenericHasNested<A2>.GenericNested<Tuple<int, bool>>).ToCSharpString();
Assert.AreEqual("GenericHasNested<A2>.GenericNested<Tuple<int, bool>>", name);
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.
Will do. Done. It's good to have a test to see whether the recursive type forwarding happens only where it should.
Beyond that, I would caution against blowing up this utility part of Windsor too much. If we wanted a really robust implementation for type name formatting, I'd be happy to donate my own recent work (stakx/TypeNameFormatter) which comes tested and as a source code NuGet package that easily embeddable (in the spirit of not reinventing the wheel and doing double work). That said, I perfectly understand if Castle would rather stay free of 3rd-party code.
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.
I did have a close look at that, it is mental the acrobatics one has to do to achieve this. I would like to avoid a third party dependency on this but ultimately it is for @jonorossi to decide. Thanks for doing this. The last test case you added was where I got up to :)
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.
I've all done it before so it wasn't too bad this time around.
Try a type such as int[][,][,,]
and see what happens to the commas. ;-)
Totally fine with not introducing a dependency, I just thought I'd mention it as a possibility.
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.
Thanks for the offer @stakx, I don't mind the contribution though would prefer it to be embedded rather than fetched from NuGet, to be embedded it probably would also be best licensed under Apache 2. However I think what we've now got covers what the debugger views are going to have to worry about anyway, I can't imagine any user of Windsor having a component of type int[][,][,,]
(can't imagine Windsor resolving it 😉), so for pragmatism sake I think your fix here is all we need.
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.
I can't imagine any user of Windsor having a component of type int[][,][,,]
You'd be surprised what contraptions people come up with when you least expect it! 😆
so for pragmatism sake I think your fix here is all we need.
Agreed, let's just wait and see if anyone complains about this again.
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.
You'd be surprised what contraptions people come up with when you least expect it! 😆
Duuuude. I feel you on that.
@stakx beat me to it! :) I had mine on a branch but did not finish it in time. |
`TypeUtil.ToCSharpString` surrounds generic type parameters with dots that noone quite seems to know what they're supposed to be good for. Let's drop those in favor of a more familiar formatting. That method also produces a rather unfamiliar formatting for nested generic types. For example, given `Outer<bool>.Inner<char>` it outputs "Outer<T>.Inner<bool, char>" (minus the centered dots) which is likely to be confusing rather than helpful. This changes the way how such types get formatted to something more familiar to C# users: "Outer<bool>.Inner<char>".
e3c4ab7
to
f4a93e1
Compare
Merged, thanks. |
This changes the way how
TypeUtil.ToCSharpString
formats certain types to a representation that looks more like C#:Generic type parameters. Those are currently surrounded with a centered dot (
·
) that noone quite seems to know what they're good for. (†) (See Resolve #405. Unicode please!!! #406, esp. the recommendation made in Resolve #405. Unicode please!!! #406 (comment).)Nested generic types. For example, given
Outer<bool>.Inner<char>
, it outputs"Outer<·T·>.Inner<bool, char>"
which is likely to be confusing rather than helpful. This changes the way how such types get formatted to something more familiar to C# users:"Outer<bool>.Inner<char>"
. (This resolves TypeUtil.ToCSharpString(this Type) does not handle nested generic types correctly #404.)These changes are classified in the changelog as an "enhancement". Given that there were explicit unit tests about that behavior, it's not really a bugfix, but a change. Classifying as a "breaking change" would be accurate, but given that the method's intended use case is for diagnostics, it seems somewhat over-the-top to mark it as "breaking".
(†) My personal theory about the centered dots is that they were needed to mark a placeholder, so you'd know how to apply, or "distribute", the generic type arguments from the innermost type over the outer types. Now that this distribution is done automatically, the dots aren't really needed anymore.