Skip to content
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

Support DateOnly and TimeOnly fields #320

Open
Turnerj opened this issue Aug 17, 2021 · 6 comments
Open

Support DateOnly and TimeOnly fields #320

Turnerj opened this issue Aug 17, 2021 · 6 comments
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement.

Comments

@Turnerj
Copy link
Collaborator

Turnerj commented Aug 17, 2021

Agreed. I have no idea how we're going to support DateOnly and have supports for the targets we do without a lot of #if statements.

Maybe... if we had an intermediary struct for dates, we could keep all the logic there rather than have a mix of the logic across the code base? Could implicitly convert between it and whatever the user intended. (Though this is also kinda messy)

Originally posted by @Turnerj in #100 (comment)

@Turnerj Turnerj added the enhancement Issues describing an enhancement or pull requests adding an enhancement. label Aug 17, 2021
@RehanSaeed
Copy link
Owner

Do we want to add this support before we release? Its potentially a breaking change.

@Turnerj
Copy link
Collaborator Author

Turnerj commented Nov 9, 2021

While it would be cool, I don't have the capacity to do this in the short term. I think we get a release out with S.T.J now and plan out exactly how we add support for these structs (whether we go with my suggestion above about intermediary structs).

@RehanSaeed
Copy link
Owner

I'm not sure it'd be that much work. I'll see if I can get to it.

https://schema.org/Date -> DateOnly
https://schema.org/Time -> TimeOnly
https://schema.org/DateTime -> DateTimeOffset
https://schema.org/Duration -> TimeSpan

@Turnerj
Copy link
Collaborator Author

Turnerj commented Nov 10, 2021

My thought was something like:

public struct SchemaDate
{
#if NET_6_OR_GREATER
    private DateOnly BackingDate;
    
    public static implicit operator DateOnly(SchemaDate schemaDate) { /* ... */ }
    public static implicit operator SchemaDate(DateOnly date) { /* ... */ }
#else
    private DateTime BackingDate;
#endif

    public static implicit operator DateTime(SchemaDate schemaDate) { /* ... */ }
    public static implicit operator SchemaDate(DateTime date) { /* ... */ }
}

And then something similar for the TimeOnly. I think something like this allows backwards compatibility and might simplify our logic handling of it or something.

If we switch out Values<,,,,> and OneOrMany<> with DateOnly and TimeOnly, we might end up having to include far more #if statements or something.

What are your thoughts on that?

@RehanSaeed
Copy link
Owner

I was hoping it'd be one or two simple if statements with an #if NET_6_OR_GREATER in it to get this to work. I'll take a look when I get a chance. Ideally, it would be good to code for .NET 6 and then fill in any #if's later as these will be deleted in a couple of years.

@Turnerj
Copy link
Collaborator Author

Turnerj commented Nov 11, 2021

Here are my quick thoughts for the scope of what I think would need changing/issues you might hit...

I think these places at minimum would need changing:

  • Source generator to use DateOnly and TimeOnly for type mapping for .NET 6 builds (I think its in the SchemaService class)
  • Replacing or modifying the DateTimeToIso8601DateValuesJsonConverter and TimeSpanToISO8601DurationValuesJsonConverter to use DateOnly and TimeOnly for .NET 6
  • A few small places in ValuesJsonConverter - mostly about reading (should be just duplicating the existing DateTime and Timespan paths) but there is also a small bit for writing Timespan values that may need a path for TimeOnly.

I don't know if the source generator is aware what the target framework is as it is a .NET Standard library so #if likely won't pick up .NET 6 when the target of the source generator is built. We probably can pass it in as a variable (similar to how we include pending types or not) though would require a little bit of extra work.

Happy to be a sounding board if you run into issues trying to add these types though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues describing an enhancement or pull requests adding an enhancement.
Projects
None yet
Development

No branches or pull requests

2 participants