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 TimeOnly, DateOnly #2596

Merged
merged 9 commits into from
Nov 14, 2021
Merged

Conversation

kerams
Copy link
Contributor

@kerams kerams commented Nov 9, 2021

Implements #2519.

TimeOnly is represented by a simple number, just like TimeSpan. DateOnly is represented by Date like DateTime.

Remaining work

  • Tests that comparison, hashing (dictionary lookup) works for both types
  • TimeOnly subtraction operator
  • TimeOnly.(Try)Parse
  • DateOnly.ToString
  • DateOnly.(Try)Parse
  • DateOnly.Add(Days|Months|Years)
  • DateOnly.ToDateTime

Not implementing

  • DateOnly has a constructor which lets you specify the calendar used to intepret the other parameters. This would add another layer of complexity, and I don't think it's of any interest to the vast majority of users.
  • DateOnly.ToLongDateString, DateOnly.ToShortDateString, TimeOnly.ToLongTimeString, TimeOnly.ToShortTimeString. You can just use ToString with the corresponding format specifier.
  • DateOnly.ToString formats D, r, R, m, M, y, Y because they produce localized text.
  • (Try)ParseExact on both types

tests/Main/DateOnlyTests.fs Outdated Show resolved Hide resolved
@alfonsogarciacaro
Copy link
Member

Great work @kerams, thank you! Please just let me know when this is ready to merge 👍

@kerams kerams marked this pull request as ready for review November 12, 2021 17:12
@kerams
Copy link
Contributor Author

kerams commented Nov 12, 2021

For some reason .NET SDK 5.0.403 was used for the last build, hence the failing tests.

@kerams kerams closed this Nov 12, 2021
@kerams kerams reopened this Nov 12, 2021
@kerams
Copy link
Contributor Author

kerams commented Nov 12, 2021

Ah, you changed the workflow file today. Well, without .NET 6 we're not going to get liftoff here :).

@alfonsogarciacaro
Copy link
Member

Sorry, we had some issues when updating to F# 6 (see #2605) but hopefully they're solved now, can you try syncing with main. Thanks!

@kerams kerams closed this Nov 14, 2021
@kerams kerams reopened this Nov 14, 2021
@kerams kerams closed this Nov 14, 2021
@kerams kerams reopened this Nov 14, 2021
@kerams
Copy link
Contributor Author

kerams commented Nov 14, 2021

@alfonsogarciacaro, I'm not sure what's going on. The new tests pass the first time around, but then do not get compiled as .NET 6 following /home/runner/work/Fable/Fable> node ./src/fable-compiler-js/src/app.fs.js tests/Main/Fable.Tests.fsproj build/tests-js. I've switched fable-compiler-js.fsproj to .NET 6, but it didn't help.

I don't think that part of the test suite had been run when I got green ticks previously.

@alfonsogarciacaro
Copy link
Member

@kerams Sorry, I should have explained this to you before. Fable standalone packs its own BCL DLLs because the point is dotnet/nuget doesn't need to be installed in the system (or the browser). These DLLs are in fable-metadata and are generated by custom build of FCS by (you probably guessed it) @ncave. We haven't updated the DLLs to net6 yet so that explains why fable-standalone doesn't find the new classes

We can update the DLLs but maybe it's better to do it when @ncave updates fcs-fable... Yet another fork to make FCS compilable by Fable :) So for now we can disable them for fable-compiler-js. I can do that so you don't need to worry.

Thanks again for this work!

@kerams
Copy link
Contributor Author

kerams commented Nov 14, 2021

Alright then, thanks.

@ncave
Copy link
Collaborator

ncave commented Nov 14, 2021

@alfonsogarciacaro I've updated the BCL to 6.0.100 in #2612, hopefully that works.

@alfonsogarciacaro alfonsogarciacaro merged commit 63c93ae into fable-compiler:main Nov 14, 2021
@kerams kerams deleted the timedateonly branch November 14, 2021 22:16
@alfonsogarciacaro
Copy link
Member

Thank you both! Released in v3.6.0-beta-001 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants