Improve docs for BinaryData.ToString()

[JoshLove-msft]

No description provided.

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[GrabYourPitchforks]

In general I don't have a concern with clarifying docs, but it does strike me as a bit weird to call out JSON string behavior specifically. Why are JSON strings promoted to a first-class citizen within the remarks section, but JSON objects/arrays aren't mentioned, and weirdness resulting from <arbitary binary data>.ToString() not called out? Is this doc change motivated by seeing how people are using / misusing this API in practice? And if that's the case, is a code analyzer a better alternative to doc updates?

[JoshLove-msft]

In general I don't have a concern with clarifying docs, but it does strike me as a bit weird to call out JSON string behavior specifically. Why are JSON strings promoted to a first-class citizen within the remarks section, but JSON objects/arrays aren't mentioned, and weirdness resulting from <arbitary binary data>.ToString() not called out? Is this doc change motivated by seeing how people are using / misusing this API in practice? And if that's the case, is a code analyzer a better alternative to doc updates?

Yes, this is motivated by users not understanding how to get a JSON string and being confused by the output from ToString. I'm not sure how an analyzer would work in general here as we would not be able to tell whether it is a valid ToString usage or not. The main goal is to just introduce the idea of ToObectFromJson<string>, as it isn't obvious to users that they would need to do this.

[ghost]

Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Author:	JoshLove-msft
Assignees:	-
Labels:	area-System.Memory
Milestone:	-

[GrabYourPitchforks]

we would not be able to tell whether it is a valid ToString usage or not.

@JoshLove-msft What would a "valid" ToString usage look like for this type? From my understanding of the type, this type is a literally a container for arbitrary binary data. That would imply that the only "correct" way to convert this to a human-readable representation is for the caller to signal what the data represents and thus what transform is valid: JSON unwrapping, UTF-8 conversion, hex conversion (for binary data), etc. I'm almost tempted to suggest marking the ToString method as [Obsolete] in addition to putting in the doc comment.

@bartonjs Do you have thoughts on this? In recent reviews you've mentioned concerns about users calling ToString on various exchange types while harboring incorrect assumptions about just what the ToString method does.

[GrabYourPitchforks]

Approved to unblock this since it's an improvement over the current situation. But per feedback in the issue conversation, I think relying solely on docs is the wrong long-term solution, and we need to think about more in-your-face mechanisms to steer users toward correct behaviors.

[JoshLove-msft]

we would not be able to tell whether it is a valid ToString usage or not.

@JoshLove-msft What would a "valid" ToString usage look like for this type? From my understanding of the type, this type is a literally a container for arbitrary binary data. That would imply that the only "correct" way to convert this to a human-readable representation is for the caller to signal what the data represents and thus what transform is valid: JSON unwrapping, UTF-8 conversion, hex conversion (for binary data), etc. I'm almost tempted to suggest marking the ToString method as [Obsolete] in addition to putting in the doc comment.

ToString does UTF-8 conversion, so any UTF-8 encoded bytes would be valid. It isn't necessarily invalid to call ToString on JSON, it is just probably not want the user wants in most cases, since there are special JSON deserialization methods on BinaryData. A case where it makes sense to use ToString would be when you are using some other textual represenation, e.g. plaintext, or XML, where BinaryData does not also have specialized methods for handling those formats. In those cases, ToString makes sense. I agree that it is somewhat ambiguous, and that this is something of a usability bandaid.

[ghost]

Hello @adamsitnik!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.

[adamsitnik]

I've applied all the suggestions and pushed them directly to @JoshLove-msft branch. LGTM

[adamsitnik]

Failures are unrelated (#56894), merging

[danmoseley]

Note this won't appear on the docs site until we run the porting tool again. If the goal is to get the changes on the doc site it's probably easiest to create another PR against dotnet/dotnet-api-docs.