ToString guidelines

[maririos]

ToString is very useful for developers, specially in debugging scenarios. As some of our customers (+ @tg-msft ) have pointed out, we’re not very consistent about adding helpful ToString methods to our types.

The purpose of this issue is to create some guidelines around the usage of ToString().
Considerations brought up by .NET team:

• How much info to include in a debug representation
• How to format it
• Whether we need to redact, possibly changing behavior around redactions if there’s a debugger attached/using DebuggerDisplay in .NET
• Make sure the ToString() doesn't change the state of the object, resulting in different behavior with and without attached debugger.

[annelo-msft]

I'd love it if we could also consider best-practices from the BCL team, i.e. if they have written guidance or tribal knowledge on this, and/or taking great examples of ToString() implementations and figuring out how we can be consistent with those in the Azure SDK.

[maririos]

Other comments from today's meeting:

Considerations:

• ToString is a way to normalize an object
• what is BCL doing? (Anne's previous comment)
  • BCL guidance: https://docs.microsoft.com/en-us/dotnet/api/system.object.tostring?view=netcore-3.1#notes-to-inheritors
• Debugger display?
• Performance shouldn't be a thing

Concerns about its use:

• People might take a dependency on it. What about a breaking change?
  Idea from @heaths : We might even put in the
  or or something for any ToString() overloads that the format is not guaranteed and may change.
• Is the problem a debugging issue? If not, is it use in logging too?

Ideas for implementation:

• An option could be that anything that a struct or has "name" on it (blobName, Id, etc) should have ToString
• Format: something standard like i.e.
  [{TypeName}: {Name}, {BigProp1}={Value1}, {BigProp2}={Value2}] So [BlobClient: http://whatever, Auth=SharedKey, Snapshot=Foo]

[maririos]

@tg-msft your format idea resonated with a comment from Chris Sells in the Framework Guidelines:

"I prefer to make my ToString output as geeky as possible to emphasize that the only “humans” that should ever see the output are “developer humans” (a subspecies all their own)."

[heaths]

In internal discussion, we can use formattable strings (e.g. $"[{TypeName}] ...") but should call ToString() on operands (or ToString(null, CultureInfo.InvariantCulture) if it implements IFormattable) ourselves for the best performance based benchmarks.

In general, I think this is a great idea where there are potentially logs of objects with clear identifying properties like an Id or Name, etc. But it doesn't make sense for all objects like clients/operations. Even some transient models like *Results models wouldn't have much value in overriding ToString, and just creates not only a burden for us to implement and support (e.g. figure out what to even display) but has the potential of people taking advantage of ToString() output even if they shouldn't e.g. using them as keys.

For example, out of 96 "System.*" assemblies I had loaded in PowerShell and of their 4,199 exported types, only 338 declared/overrode ToString():

$assemblies = [AppDomain]::CurrentDomain.GetAssemblies() | where FullName -like 'System.*'
$assemblies | measure
# 96
$allTypes = $assemblies.GetExportedTypes()
$allTypes | measure
# 4,199
[type[]]$empty = @()
$allTypes | foreach { $_.GetMethod('ToString', 'Public,Instance,DeclaredOnly', $null, $empty, $null) } | measure
# 338

[tg-msft]

We should have separate suggestions for our *Client types that help make it easy to identity the resource we're pointing at.

For example, [BlobClient Uri=http://acct1.blob.core.windows.net/container2/blob3]. I think the Uri/Endpoint/whatever we call it is a good starting point. We could also compare with breaking it up like [BlobClient Account=acct1, Container=container2, Blob=blob3]. I don't think we want the service version but we might want the auth mechanism used (depending on the outcome of Azure/azure-sdk#1436) like [BlobClient Uri=http://acct1.blob.core.windows.net/container2/blob3, AuthenticationMode=SharedKey].

[maririos]

As for Models, during today's meeting some of the ideas suggested where:

• Include all the properties that are required
• Identify the key (main) properties -> unless names like Id, or *Name, some manual work will need to happen
• Ignore default values that have not been changed by user

[maririos]

From @KrzysztofCwalina perspective, in general, it will be good to have ToString in each client/model but if it involves too much work, then don't do it

[maririos]

I will start by looking into ToString for our next set of TextAnalytics releases and will come back here with ideas/suggestions/questions ... etc

[MiYanni]

This might help. I did an automatic ToString implementation in the past for logging. I defined a format and used reflection to build a consistent string representation of any type automatically. It was extremely useful in logging exceptions:
https://github.com/MiYanni/Keyboard-Mapper/blob/3fb69d7d78b9933fe2d2fa54659753d6a0f7f823/Common/Extensions/StringExtensions.cs#L80-L90

I even have tests for it! https://github.com/MiYanni/Keyboard-Mapper/blob/3fb69d7d78b9933fe2d2fa54659753d6a0f7f823/Common/Extensions/StringExtensionsTests.cs#L184-L189

[pallavit]

Given we are moving to Typespec and will get the consistency by default for all our generated SDKs I believe any guidelines we want our SDKs to follow should be added to the generation logic directly.

Looking at the conversation - as of now the bug seems exploratory and I don't see any consistent ToString() implementation that we would want our generators to follow. Please let me know if my understanding is incorrect.

Do we have a tag that we use for experimentation? It is not MQ and not necessarily feature_req but I can see it as an "feature request" for our SDKs? Is there any other tag that will suit this?

/cc: @jsquire

[annelo-msft]

@pallavit, I believe this was a guidelines issue and it might make sense to move it to the azure-sdk repo given that. I agree that implementation of the guidance, once it's established, would likely fall to the generator where appropriate, unless it refers to Core types or other manually-written types.

[pallavit]

What kind of guideline(s) are we looking for here? Do we want guidance from architects on what we should do with ToString() APIs , whether we should have them or what information would be useful for our customers?
I have not seen any issues\customer feedback around this so was curious what would be consider success for this issue?

[heaths]

Yes, we are primarily looking for architect guidelines on when we should override ToString. @maririos's OP, I think, laid out the scenarios pretty well. Debugging is a big use case for overriding ToString, though there are other ways to achieve the same thing but with more work and often not a lot of value otherwise if overriding ToString is sufficient. Having a depate DebugViewAttribute and supporting class is useful when you really need ToString and debugging views to differ, which should be rare for us.

[JimSuplizio]

Hi @maririos, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.