moq/labs

`Stringly.cs` doesn't format all types correctly

Closed this issue · 10 comments

stakx commented

This is probably low-priority, as it might affect mostly diagnostic messages, and not Moq's core functionality.

The Stringly class in both Stunts and Moq.Sdk supposedly formats Type objects to a C# representation, but there are several cases where it doesn't work as it should. Here's just a few examples (there are probably more):

class A { }

class A<T>
{
    public class B<U> { }
}

[Theory]
[InlineData( typeof(A[][,]),           "A[][,]"           )] // => actual: "A[,][]"
[InlineData( typeof(A<object>[]),      "A<object>[]"      )] // => actual: "A`1[]"
[InlineData( typeof(A<>),              "A<>"              )] // => actual: "A`1"
[InlineData( typeof(A<object>.B<int>), "A<object>.B<int>" )] // => actual: "A`1.B<object>"
public void Stringly_ToTypeName_formats_correctly(Type type, string expected)
{
    Assert.Equal(expected, Stringly.ToTypeName(type));
}

I'm aware that Stringly probably doesn't have to be perfect, and that you might not be interested in fixing every single corner case (many might be relevant in a mocking scenario).

However, if you do want to improve Stringly: I'm currently working on a standalone library to do much the same of what Stringly does; see stakx/TypeNameFormatter. It covers the above corner cases, and then some more. I'm aiming at turning this into a NuGet package eventually (possibly a source-only one). If you're interested in reusing this effort in some way, let me know.

kzu commented

I'd love a source-only package so we can get rid of that baggage that's not core to moq 💯 !

stakx commented

I'd love a source-only package so we can get rid of that baggage that's not core to moq

Done! I've made a source-only package available as a pre-release on NuGet.

using TypeNameFormatter;

// See project README for a quick overview over the available options.
// The default yields the most compact formatting possible.
var options = TypeNameFormatOptions.Default;

// Format a type name using a throw-away StringBuilder and return a string.
// Specifying `options` is optional.
string name = typeof(T).GetFormattedName(options);

// Or, append a type name to an existing StringBuilder.
// (Again, you don't need to specify `options` explicitly.)
stringBuilder.AppendFormattedName(typeof(T), options);

I'd love if you could give it a try and see whether this works for you.

Hopefully, the source-only packaging is done right. I tried to follow the approach you used in IFluentInterface but I'm not the MSBuild wizard you are. I originally started out with the approach you used in IFluentInterface, but as it turned out this didn't work so well in JetBrains Rider or with ReSharper, so I've since chosen a different approach in the .targets file.

Any feedback is greatly appreciated!

kzu commented

Hm, how is the IFluentInterface not working on Rider or Resharper? :S

stakx commented

@kzu, basically, unlike Visual Studio, Rider does not process <Compile Include="..." /> when they are defined inside a <Target>... in that case, the included files won't be included in "design-time" code analysis. This means that Rider IDE will complain about the IFluentInterface not being defined. Things will still compile fine, it's just IDE code analysis that won't know about the included source file.

I've rewritten TypeNameFormatter's .targets file to include the package's single source file outside of any <Target>, then I'm just conditionally hiding it from the IDE using the <Visible> item attribute. See https://github.com/stakx/TypeNameFormatter/blob/f55ae920dd399887340b0e33eaaecd1fea0a92e5/src/TypeNameFormatter/TypeNameFormatter.Sources.targets#L36-L46.

stakx commented

(Secondly, somewhat unrelated but still good to know, Rider will display all MSBuild items as project items in the IDE's "solution explorer" (unlike Visual Studio which will only display elements of certain well-known items like <Compile>). This may be undesirable if you're using "temporary" item groups. So you probably want to limit use of item groups in order to not populate Rider's "solution explorer" with intermediary files. That's why I'm hard-coding and repeating the same file name references for TypeNameFormatter.cs everywhere, instead of the cleaner way of putting it in an item group.)

kzu commented

Well, both look like bugs on Rider that will make it choke on the vast majority of C# projects out there since relying on CoreCompile and Compile items added via their targets is quite common I'd say.

I'd rather just have people nag the Jetbrains guys to fix their IDE to be more compatible with VS, instead of working around their bugs/missing features ¯_(ツ)_/¯.

Do you know if that bug is being tracked on their side somewhere?

kzu commented

Also, have you tried the <InProject>false</InProject> metadata to see if it works? That's also a supported mechanism in both VS and VSfM...

stakx commented

I'd rather just have people nag the Jetbrains guys to fix their IDE to be more compatible with VS, instead of working around their bugs/missing features ¯_(ツ)_/¯.

I agree. This would be preferable to bending over backwards to support several incompatible project systems.

Do you know if that bug is being tracked on their side somewhere?

No, I haven't checked. I'm using Rider rarely (if ever) so for me it was easier at the time to just rewrite the MSBuild .targets file than having to follow up on a bug report over a longer period of time.

In other words, I was too lazy to report it. (But like I said above, I think it would be a good idea if someone did this.)

Also, have you tried the <InProject>false</InProject> metadata to see if it works?

IIRC, I did try this but it didn't make a difference in Rider. (I'd verify that claim quickly but I don't have a Rider installation right now... sorry!)

stakx commented

Closing this as you've just replaced Stringly. I might open a ticket over at TypeNameFormatter regarding the Rider "incompatibility" with how VS handles project files (discussed above).

kzu commented

Yep, I think they will likely fix it as more people push for it