Show properties as well as values when formatting `IEnumerable` types #3074

jonsequitur · 2023-07-07T21:57:15Z

This fixes the following issues:

colombod · 2023-07-08T00:31:51Z

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter{T}.cs

@@ -186,7 +192,17 @@ bool BuildTable(T source, FormatContext context, bool summarize)
                    }
                }

-                rows.Add(tr(rowValues.Select(r => td(r))));
+                rows.Add(tr(rowValues.Select(value =>


do we need any change on the TabularDataResource Formatter?

What kind of change?

shyamnamboodiripad · 2023-07-10T19:29:31Z

src/Microsoft.DotNet.Interactive.Formatting/Formatter.cs

@@ -309,6 +309,7 @@ public static void Format(this ITypeFormatter formatter, object instance, TextWr
    public static void FormatTo<T>(
        this T obj,
        FormatContext context,
+        // FIX: (FormatTo) make this non-optional


Did you intend to remove this before merge?

Yeah I suppose I might as well do this now. It's a breaking change that eventually has to happen.

shyamnamboodiripad · 2023-07-10T19:37:47Z

src/Microsoft.DotNet.Interactive.Formatting/Formatter.cs

+                if (@interface.GetGenericTypeDefinition() == typeof(IDictionary<,>))
+                {
+                    return false;
+                }


Just curious: Why only IDictionary and IOrderedEnumerable specifically?

These interfaces have properties that aren't very interesting, e.g. Count (since we already show it for all sequences.)

shyamnamboodiripad · 2023-07-10T19:42:57Z

src/Microsoft.DotNet.Interactive.Formatting/Formatter.cs

+            return false;
+        }
+
+        if (typeof(ICollection).IsAssignableFrom(type))


Curious why properties are included for types that implement IEnumerable but not ICollection.

ICollection is a lot more specific and includes a Count property which is redundant with the existing sequence formatters, which show e.g. (13 more) after hitting the list expansion limit.

shyamnamboodiripad · 2023-07-10T19:46:56Z

src/Microsoft.DotNet.Interactive.Formatting/Formatter.cs

+            return false;
+        }
+
+        if (typeof(ICollection).IsAssignableFrom(type))


This may be a micro-optimization, but considering that we are already looking at implemented interfaces in the loop below, would it be better to add an early out for ICollection there instead? (Taking a quick look at the implementation for IsAssignableFrom it seems to be doing a bunch morethings before checking if an implemented interface IsSubClassOf(typeof(ICollection)) )

Moving it into the loop means we're paying the cost of GetTypeInfo and then doing an equality check against ICollection for every single interface we find. This isn't to say that I think that's expensive, I just don't have enough information to say whether this would be an optimization or actually be more costly.

shyamnamboodiripad · 2023-07-10T19:50:43Z

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter.cs

@@ -43,13 +42,21 @@ void Initialize()

    internal static void FormatAndStyleAsPlainText(
        object value,
-        FormatContext context)
+        FormatContext context,
+        string mimeType = PlainTextFormatter.MimeType)


It is a bit confusing to see the PlainText suffix in the name of the function when the mime type is also specified in this parameter. Could we do something to disambiguate?

(Also applies to TagWithPlainTextStyling() helper below)

Any suggestions?

"style as plain text" seems reasonable in the context of the HtmlFormatter.

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter.cs

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter{T}.cs

shyamnamboodiripad · 2023-07-10T20:25:35Z

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter{T}.cs

+                            enumerableFormatter.Format(obj, c);
+                        }));
+                Array.Resize(ref rows, rows.Length + 1);
+                rows[^1] = enumerableAccessor;


minor: Would it be better to leave rows as an enumerable and use Enumerable.Append here (and call ToArray right before calling BuildTreeView below)?

I would assume that would be less performant than doing the array operations directly, though it would be far from the most critical piece of code to optimize here.

shyamnamboodiripad · 2023-07-10T20:31:18Z

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter.cs

@@ -121,7 +128,7 @@ void Initialize()

        new HtmlFormatter<string>((s, context) =>
        {
-            // If PlainTextPreformat is true, then strings
+            // If PreformatPlainText is true, then strings


Is PreformatPlainText referring to an option that is present inside context?

Deleting this comment. It was outdated and refers to something that no longer exists.

shyamnamboodiripad · 2023-07-10T20:33:09Z

src/Microsoft.DotNet.Interactive.Formatting/PlainTextFormatter{T}.cs

@@ -107,10 +107,14 @@ internal static PlainTextFormatter<T> CreateForAnyEnumerable()

        return new((value, context) =>
        {
+            using var a = context.IncrementDepth();
+            using var b = context.IncrementDepth();


The variables (a and b) seem to be unused - I am guessing you added them for testing and intended to remove them before merge.

They are being used because we're using them. 😂 (Sorry, that pun was right there.) In other words, Dispose is being called on them.

The variable names are awful.

I often use _ as a variable name for these situations. I suppose _ and __ can work.

Ah I missed the using keyword 😄

shyamnamboodiripad · 2023-07-10T20:35:28Z

src/Microsoft.DotNet.Interactive.Formatting/PlainTextSummaryFormatter.cs

@@ -8,6 +8,7 @@ namespace Microsoft.DotNet.Interactive.Formatting;

 public static class PlainTextSummaryFormatter
 {
+    // FIX: (PlainTextSummaryFormatter) rename this
    public const string MimeType = "text/plain+summary";


Again, I am guessing you intend to fix this up before merge. If not, I wonder if it may be better to log an issue to track this...

That's a bigger change because it affects the VS Code extension. I've opened #3078 to track it.

shyamnamboodiripad · 2023-07-10T20:52:08Z

src/Microsoft.DotNet.Interactive.Formatting.Tests/Utility/StringExtensions.cs

+        {
+            return XElement.Parse(html).ToString();
+        }
+        catch (XmlException ex) when (ex.Message.Contains("multiple root elements"))


Would we ever want to run tests on localized / non-ENU setups?

It's unlikely but if so, it will produce a false negative rather than a false positive, which is good, and it will likely be pretty clear what happened. I did look for a better programmatic way to determine the specific cause of the XmlException but I didn't find anything.

jonsequitur requested review from colombod and shyamnamboodiripad July 7, 2023 21:57

colombod reviewed Jul 8, 2023

View reviewed changes

shyamnamboodiripad reviewed Jul 10, 2023

View reviewed changes

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter.cs Show resolved Hide resolved

shyamnamboodiripad reviewed Jul 10, 2023

View reviewed changes

src/Microsoft.DotNet.Interactive.Formatting/HtmlFormatter{T}.cs Show resolved Hide resolved

shyamnamboodiripad reviewed Jul 10, 2023

View reviewed changes

jonsequitur added 10 commits July 10, 2023 13:52

wip

f3ddaa1

wip

74b9ccb

remove unused XML doc

3f60d65

wip

5e97ecf

wip

4963d4b

wip

9f0d2ec

move Join methods to PlainTextFormatter, include properties in output

b5761ed

fix Powershell test

87be9aa

make mimeType parameter non-optional in FormatTo

c196bf0

fix test

a42d0dc

jonsequitur force-pushed the formatter-fixes-for-IEnumerable-with-properties branch from b8171ac to a42d0dc Compare July 10, 2023 20:52

shyamnamboodiripad approved these changes Jul 10, 2023

View reviewed changes

jonsequitur added 2 commits July 10, 2023 14:19

PR comments

270b626

API baseline updates

9ac7ceb

jonsequitur enabled auto-merge (rebase) July 10, 2023 21:20

jonsequitur disabled auto-merge July 10, 2023 21:20

jonsequitur enabled auto-merge (squash) July 10, 2023 21:21

jonsequitur merged commit d87e3e1 into dotnet:main Jul 10, 2023
4 checks passed

colombod added the Area-Formatting Data and object formatting as HTML and plaintext label Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show properties as well as values when formatting `IEnumerable` types #3074

Show properties as well as values when formatting `IEnumerable` types #3074

jonsequitur commented Jul 7, 2023

colombod Jul 8, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023 •

edited

Loading

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023 •

edited

Loading

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

shyamnamboodiripad Jul 10, 2023

jonsequitur Jul 10, 2023

Show properties as well as values when formatting IEnumerable types #3074

Show properties as well as values when formatting IEnumerable types #3074

Conversation

jonsequitur commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamnamboodiripad Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Show properties as well as values when formatting `IEnumerable` types #3074

Show properties as well as values when formatting `IEnumerable` types #3074

shyamnamboodiripad Jul 10, 2023 •

edited

Loading

shyamnamboodiripad Jul 10, 2023 •

edited

Loading