Skip to content
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

Add nullness warnings in JsonParse #9595

Merged
merged 5 commits into from Mar 4, 2024
Merged

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Feb 6, 2024

Summary

Today I discovered the surprising fact that JsonNode.Parse(x).ToJsonString() may throw on valid inputs. It seems reasonable to document this.

@Smaug123 Smaug123 requested a review from a team as a code owner February 6, 2024 20:09
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 6, 2024
@Smaug123 Smaug123 force-pushed the json-nodes branch 2 times, most recently from da3145b to 2ad44d9 Compare February 6, 2024 20:11

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

@Smaug123
Copy link
Contributor Author

Smaug123 commented Feb 6, 2024

What's the correct way to link to a method when I don't care which overload I'm linking to? The bot appears to be complaining that M:System.Text.Json.Nodes.JsonNode.Parse doesn't exist. M:System.Text.Json.Nodes.JsonNode.Parse(System.IO.Stream,System.Nullable{System.Text.Json.Nodes.JsonNodeOptions},System.Text.Json.JsonDocumentOptions) does exist (I can see its doc ID in the docs), but I specifically don't want to link to a particular overload.

@MSDN-WhiteKnight
Copy link
Contributor

What's the correct way to link to a method when I don't care which overload I'm linking to?

It should be M:System.Text.Json.Nodes.JsonNode.Parse%2A (%2A stands for *)

This comment was marked as outdated.

@MSDN-WhiteKnight
Copy link
Contributor

Looks like this feature does not work with cref. Try converting remarks to markdown (like there: https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Reflection.Metadata/MetadataReader.xml#L40) and use xref syntax:

<xref:System.Text.Json.Nodes.JsonNode.Parse%2A>

Copy link

Learn Build status updates of commit a884853:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Text.Json.Nodes/JsonNode.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Mar 1, 2024

Looks like this feature does not work with cref. Try converting remarks to markdown (like there: https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Reflection.Metadata/MetadataReader.xml#L40) and use xref syntax:

<xref:System.Text.Json.Nodes.JsonNode.Parse%2A>

You can link to the overloads page using cref (see https://learn.microsoft.com/contribute/content/dotnet/api-documentation#cross-references):

<see cref="Overload:System.Text.Json.Nodes.JsonNode.Parse" />

@Smaug123
Copy link
Contributor Author

Smaug123 commented Mar 2, 2024

Is this something you would prefer me to do instead of what's currently on the branch? The round-trip time is very long and I can't even see the result.

@gewarren
Copy link
Contributor

gewarren commented Mar 2, 2024

Is this something you would prefer me to do instead of what's currently on the branch? The round-trip time is very long and I can't even see the result.

I updated it. I would also like @eiriktsarpalis to review this PR before we merge it.

This comment was marked as outdated.

Copy link

Learn Build status updates of commit aac16ce:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Text.Json.Nodes/JsonNode.xml ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@gewarren gewarren merged commit 4322ffd into dotnet:main Mar 4, 2024
3 checks passed
@Smaug123 Smaug123 deleted the json-nodes branch March 4, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants