Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixes #2293: Enable DateOnly and TimeOnly #3078
Fixes #2293: Enable DateOnly and TimeOnly #3078
Changes from 1 commit
d8e9581
fb8086f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it's necessary to cast to
Date
here? Could we consider creating Date-to-string helper function that could be used to ensure consistency for bothDate
andDateOnly
but could allow us to work withDateOnly
without having to cast toDate
? I think ifDateOnly
is used often or the preferred choice in the future, we should consider having first-class support for it and avoid the overhead of casting toDate
when possible. If that's a lot of work, it could be done in a follow-up PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the next breaking change version (9 version), we should remove 'Date' 'TimeOfDay' and use 'DateOnly' and 'TimeOnly' types introduced in .NET.
In current 8 version, I try to avoid breaking change. And most important, we should make sure to get the same 'literal' string no matter whether customers are using 'Date' or 'DateOnly'.
DateOnly has several 'ToString()' overloads, the string literal varys along with the formatter provider. 'Cast' to Date then call 'ToString' on 'Date' is a 'center' method and make sure we get the same literal string no matter what type is. And to call 'ToString()' is a straightforward. There's no need to create a helper method to wrapper 'ToString()' again. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious about the potential overhead of casting to
Edm.Date
so I ran some tests to measure theToString()
scenario. To my surprise, casting toEdm.Date
then callToString()
(i.e.((Date)dateOnly).ToString()
appears to be faster thandateOnly.ToString("yyyy-MM-dd", InvariantCulture)
.This issue appears to be specific case of
DateOnly.ToString(format, provider)
overload which verifies that format is a valid date-only format, and this verification happens in O(n). You can bypass this overhead by converting theDateOnly
toDateTime
then callingDateTime.ToString("yyyy-MM-dd", InvariantCulture)
. This happens to be faster than casting toEdm.Date
.Here's the benchmark code.
This was meant to verify whether there's an actual overhead of casting
DateOnly
toEdm.Date
and that we can avoid that perf overhead by handlingDateOnly
directlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar concerns as https://github.com/OData/odata.net/pull/3078/files#r1792817270 on whether we can avoid casting to
TimeOfDay
and handleTimeOnly
directly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When passing primitive values like this (
true
,false
) to arguments of a method, consider explicitly adding the argument name for readability.I know this is like this for the rest of the class but consider that as a refactor outside of this PR too.