Skip to content

Conversation

rpetrusha
Copy link

Fixed broken xref in Utf8JSonWriter

@rpetrusha rpetrusha added this to the May 2019 milestone May 8, 2019
@rpetrusha rpetrusha self-assigned this May 8, 2019
@mairaw mairaw requested a review from ahsonkhan May 10, 2019 19:11
Since this type is a ref struct, it does not directly support async. However, it does provide support for reentrancy to write partial data and to continue writing in chunks.
To be able to format the output with indentation and white space OR to skip validation, create an instance of <xref:System.Text.Json.JsonWriterState> and pass that in to the writer.

To be able to format the output with indentation and white space OR to skip validation, create an instance of <xref:System.Text.Json.JsonWriterOptions> and pass it in to the writer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the only change that would need to be made. The type is no longer a ref struct, and a few APIs/comments changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably re-run the tool to grab comments from source similar to what @carlossanlop had done previously (#2360)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team, I re-ran the tool and opened a new PR with the most up to date API versions: #2471

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR doesn't contain this change so do we need to run the tool replacing existing content too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the case of "merge-conflict (case 2 below)" which means we need to manually update the contents to match the new content.

Here are the cases:

  1. Source contains comments, docs repo doesn't - @carlossanlop tool captures the diff and adds the content.
  2. Source contains comments, docs contains comments - usually do nothing (the tool ignores this), docs are the source of truth (unless an update was required/API change/etc - then manual updates needed).
  3. Source doesn't contain comments, docs repo contains comments - do nothing, we have the docs already.
  4. Source doesn't contain comments, docs repo doesn't contain comments - dev/docs team should add comments to this repo.

Copy link
Contributor

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but left a comment re. the source comments for @ahsonkhan and @carlossanlop

Since this type is a ref struct, it does not directly support async. However, it does provide support for reentrancy to write partial data and to continue writing in chunks.
To be able to format the output with indentation and white space OR to skip validation, create an instance of <xref:System.Text.Json.JsonWriterState> and pass that in to the writer.

To be able to format the output with indentation and white space OR to skip validation, create an instance of <xref:System.Text.Json.JsonWriterOptions> and pass it in to the writer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR doesn't contain this change so do we need to run the tool replacing existing content too?

@rpetrusha rpetrusha merged commit ce08820 into dotnet:master May 23, 2019
@rpetrusha rpetrusha deleted the json-link branch May 23, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants