Skip to content

Commit 7fca47a

Browse files
authored
Merge pull request #1716 from fsprojects/repo-assist/json-codegen-dead-code-cleanup-1d067e122892e0b5
[Repo Assist] docs: clarify reachability of TODO branches in JSON code generator
2 parents 2bdce04 + e442fdf commit 7fca47a

2 files changed

Lines changed: 13 additions & 4 deletions

File tree

src/FSharp.Data.DesignTime/Json/JsonConversionsGenerator.fs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,15 +100,19 @@ let internal convertJsonValue
100100
(<@ (%varExpr).Path() @>, convert <@ Some (%varExpr).JsonValue @>, <@ Some (%varExpr).JsonValue @>))
101101
| TypeWrapper.Option, true -> convert <@ (%%value: JsonValue option) @>
102102
| TypeWrapper.Option, false ->
103-
//TODO: not covered in tests
103+
// Reached when canPassAllConversionCallingTypes=false (e.g. for array element types or
104+
// heterogeneous type variants). The caller passes an IJsonDocument; we extract its
105+
// JsonValue and wrap it in Some before handing it to the conversion quotation.
104106
convert <@ Some (%%value: IJsonDocument).JsonValue @>
105107
| TypeWrapper.Nullable, true ->
106-
//TODO: not covered in tests
108+
// TypeWrapper.Nullable is never produced by JSON inference (which only emits None or Option).
109+
// This branch exists for structural completeness and potential future extensibility.
107110
typeof<TextRuntime>?(nameof (TextRuntime.OptionToNullable))
108111
(field.RuntimeType)
109112
(convert <@ (%%value: JsonValue option) @>)
110113
| TypeWrapper.Nullable, false ->
111-
//TODO: not covered in tests
114+
// TypeWrapper.Nullable is never produced by JSON inference (which only emits None or Option).
115+
// This branch exists for structural completeness and potential future extensibility.
112116
typeof<TextRuntime>?(nameof (TextRuntime.OptionToNullable))
113117
(field.RuntimeType)
114118
(convert <@ Some (%%value: IJsonDocument).JsonValue @>)

src/FSharp.Data.DesignTime/Json/JsonGenerator.fs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,12 @@ module JsonTypeBuilder =
632632

633633
match propResult.OptionalConverter with
634634
| Some _ ->
635-
//TODO: not covered in tests
635+
// Reached when a property is optional (IsOptional=true) but its
636+
// ConversionCallingType is JsonDocument (optionalityHandledByProperty=false),
637+
// meaning the property type has a converter but optionality is not handled
638+
// by the conversion calling type alone. In practice this requires an
639+
// InferedType.Json(optional=true) property, which the JSON provider does
640+
// not currently produce.
636641
ctx.JsonRuntimeType?(nameof (JsonRuntime.ConvertOptionalProperty))
637642
(propResult.ConvertedTypeErased ctx)
638643
(jDoc, propName, propResult.ConverterFunc ctx)

0 commit comments

Comments
 (0)