-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
TimeOnlyConverter
should tolerate more formats in line with TimeSpanConverter
.
#105892
Comments
What exactly led you to making this statement? Nullable DateOnly and TimeOnly are just some concrete |
I got an exception, I'll work on reproducing it on Monday |
I misread the exception, it's not that Nullable TimeOnly isn't supported, it's the specific value that it can't convert to a Nullable TimeOnly. The value it's trying to convert to Nullable TimeOnly is "16:55" which seems like it should be valid (given that TimeOnly has a constructor that only needs hour and minute) but the serializer will only accept "16:55:00". The exception I get is
|
The converter for Line 13 in 6931b3b
|
OK, well if it's intended behavior then we can close this |
Don't close, yet. This seems to be a bug, an oversight, or still a pending work item as far as STJ 9.0.0 is concerned. I just tested with STJ 9.0.0-preview.6.24327.7. It can deserialize |
The comments in the code for TimeSpan for JSON deserialization also appear to require seconds: Lines 22 to 25 in 6931b3b
|
PEBCAK. PR #102091 enables TimeSpan deserialization without seconds without problems. It's just that TimeOnly deserialization isn't benefiting from this improvement, yet... |
In that case, this issue is a feature request to support AFAIK, it's too late for such a change to be included in .NET 9 if approved, it would be part of .NET 10. |
@martincostello FYI, the code comment you referred to is inaccurate when the |
@eiriktsarpalis Can you weigh in on if you think we should address this, and if so, in .NET 9 during RC1/RC2? |
Extending #102091 to also apply to the built-in @ghosttie you mentioned in your initial post that |
TimeOnlyConverter
should tolerate more formats in line with TimeSpanConverter
.
No, sorry, I haven't tried this with DateOnly, I modeled the title off of #53539 because of my incorrect assumption about what the problem was |
Actually I've just had an exception with Nullable DateOnly
when trying to deserialize the value '' as a Nullable DateOnly I guess this is a separate issue from TimeOnly not working without seconds |
I'm not sure that's valid - an empty string isn't null and it isn't a date, so I'd argue that's the correct behaviour to fail to deserialize. |
In #53539 support for DateOnly and TimeOnly was added to JsonSerializer but nullable versions of them is not supported
The text was updated successfully, but these errors were encountered: