Update stage 0 and adjust to s.t.json api #11193
Conversation
|
|
||
| public string ToJson() | ||
| { | ||
| var state = new JsonWriterState(options: new JsonWriterOptions { Indented = true }); |
There was a problem hiding this comment.
@ahsonkhan i need to get this in by the end of friday. Hope you can look at it soon. Thanks!
|
@dsplaisted I find s.t.json api breaking change at the same time, but this should prove our original bug is not longer repro |
|
@johnbeisner Sorry nvm. I forget to update LKG version |
| writer.WriteEndObject(); | ||
| writer.WriteEndObject(); | ||
| writer.Flush(true); | ||
| writer.Flush(); |
There was a problem hiding this comment.
nit: add the following after the call to flush, if you want to be super paranoid (but it isn't necessary).
Debug.Assert(writer.CurrentDepth == 0);There was a problem hiding this comment.
Thanks! I try to minimize debug vs release difference. So I will just keep it like that
There was a problem hiding this comment.
I don't necessarily see why you wouldn't use Debug.Asserts to ensure things you expect to be invariant (just to catch possible oversight in source), but that's fine. Ideally this should be caught in your unit tests anyway.
It's already used all over the place in this repo, which is expected. https://github.com/dotnet/cli/search?q=debug.assert&unscoped_q=debug.assert
There was a problem hiding this comment.
Btw, for context, the reason why I brought it up is because Flush() doesn't guard against un-closed start object/array delimiters.
So you could have done write.StartObject and then forget to call write.EndObject. Adding such a debug.assert just validates correct depth for you and would catch accidental missing calls (especially if you pass the writer along and have deeper call graphs). FWIW.
|
LGTM |
3 participants
No description provided.