Uh oh!
There was an error while loading. Please reload this page.
Extend JsonSourceGenerationOptionsAttribute to have feature parity with JsonSerializerOptions. - #88753
Conversation
ghost
commented
Jul 12, 2023
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
ghost
commented
Jul 12, 2023
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFix #57321. Pending API approval.
|
a93f460 to
2eea328CompareAPI has been approved as proposed and this PR is ready for review. |
…th JsonSerializerOptions.
2eea328 to
8c76596Compare| }; | ||
| """); | ||
| return policyName != null |
There was a problem hiding this comment.
is not null is usually better but we haven't been consistent in its use. In this case it's fine to use != since the particular type doesn't define any custom operators.
| { | ||
| var converterType = (ITypeSymbol?)element.Value; | ||
| TypeRef? typeRef = GetConverterTypeFromAttribute(contextType, converterType, contextType, attributeData); | ||
| if (typeRef != null) |
| break; | ||
| case nameof(JsonSourceGenerationOptionsAttribute.MaxDepth): | ||
| maxDepth = (int)namedArg.Value.Value!; |
There was a problem hiding this comment.
Unhandled exceptions will get caught by the compiler and surfaced as a warning along the lines of "the source generator threw an exception and will not generate any code".
eiriktsarpalis
commented
Jul 14, 2023
Build failures not related to the change. |
Uh oh!
There was an error while loading. Please reload this page.
| /// <summary> | ||
| /// The default buffer size in bytes used when creating temporary buffers. | ||
| /// </summary> | ||
| public int DefaultBufferSize { get; set; } |
There was a problem hiding this comment.
By default this is going to be 0. Doesn't that differ from JsonSerializerOptions.DefaultBufferSize, which defaults to a positive value and throws an exception if it's set to 0?
There was a problem hiding this comment.
Because the attribute is only being read by Roslyn, the value of the property is only being consulted if explicitly set by the user in the attribute declaration. I briefly considered making this and other properties nullable, however that would be a departure from convention already used in properties shipped in this attribute, so went with consistency instead. The other alternative would be to explicitly set the properties to equal the JsonSerializerOptions default, however that would necessitate a bit of duplication or refactoring without providing any clear benefit.
There was a problem hiding this comment.
Then shouldn't the XML comment call out what a value of 0 means, as is done below for MaxDepth?
There was a problem hiding this comment.
I would probably opt for removing that reference in MaxDepth, making all of them deliberately ambiguous and just have them point to the corresponding JsonSerializerOptions properties.
There was a problem hiding this comment.
We need to call out somewhere in the docs what it means to not provide a value for these.
There was a problem hiding this comment.
I'll follow up with a PR once we conclude P7 work.
| public Type[]? Converters { get; set; } | ||
| /// <summary> | ||
| /// The default buffer size in bytes used when creating temporary buffers. |
There was a problem hiding this comment.
These comments on properties inconsistently start or don't start with "Gets or sets..."
There was a problem hiding this comment.
I mostly just copied and pasted the wording in the corresponding JsonSerializerOptions properties. I can submit a follow-up PR that improves consistency between properties in this attribute.
Uh oh!
There was an error while loading. Please reload this page.
Fix#57321.