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

Temporal bindings #76

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Temporal bindings #76

wants to merge 12 commits into from

Conversation

tom-sherman
Copy link
Contributor

@tom-sherman tom-sherman commented Dec 30, 2021

https://tc39.es/proposal-temporal/docs

Currently incomplete, need to clean the rest up before committing them.

Couple of questions on approach:

  • Is there a way to more directly type functions that type option bags? I assume no and we have to wait for this RFC
  • Many modules share a lot of functions, is it worth segmenting these out into functors? Or is a more direct approach, as in this PR currently, more valuable?

I've skipped APIs that rely on bigints, I think official support need to land for these (at least in the form of a type in the stdlib) before adding bindings. See rescript-lang/rescript-compiler#4677

I've also skipped toLocaleString bindings as I think we should add general Intl bindings first (#77)


@new
external make: (
~isoYear: int,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if these (as well as the params for PlainTime.make) should be prefixed with iso, what do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The from methods don't prefix the keys with iso. So I think even though the spec describes them as isoYear etc we should stay consistent and not prefix the parameters.


@scope("Temporal.ZonedDateTime") @val external fromString: string => t = "from"
@scope("Temporal.ZonedDateTime") @val
external fromStringWithOptions: (string, FromOptions.t) => t = "from"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken different approaches for option bags depending on the optionality of the properties. My logic goes as follows:

  • If there is only one property, use an object type. Mark it as required even if the API accepts an empty object
  • If there is more than one property but they're all required, use an object type
  • If there is more than one property and one or more are optional, use an option bag module

Is this consistent with other bindings? If so, would be good to document this in some style guide.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record types are best when all properties are required, imo. They have the advantage of looking like objects when calling the function; the @obj system was created a long time ago and is now only used as a bit of a hack to omit unspecified properties instead of setting them to undefined.

And yes now that we're getting more contributors a style guide is probably worth starting. It might make a nice binding cheat-sheet too.

@TheSpyder
Copy link
Owner

TheSpyder commented Dec 31, 2021

Regarding BigInt, maybe we consider pulling in rescript-js. That might be a bit heavy-handed, though, to force webapi users to adopt the alternative Js implementation as well if they want to make use of the BigInt functions.

@@ -0,0 +1,22 @@
module DifferenceOptions = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This submodule doesn't seem to add anything? I think it would work just as well without the submodule wrapper.

Comment on lines +55 to +68
module RoundOptions = {
type t

@obj
external make: (
~smallestUnit: roundTo,
~roundingIncrement: int=?,
~roundingMode: [#halfExpand | #ceil | #trunc | #floor]=?,
unit
) => t = ""
}

@send external round: (t, ~to: roundTo) => t = "round"
@send external roundWithOptions: (t, RoundOptions.t) => t = "round"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're careful the sub-module can be avoided for simple cases like this. I know the rest of the library does it this way but once we look at modernising the API (and reducing runtime code generation) I might try to rename some of the make submodule methods to be named directly, instead of encapsulated:

Suggested change
module RoundOptions = {
type t
@obj
external make: (
~smallestUnit: roundTo,
~roundingIncrement: int=?,
~roundingMode: [#halfExpand | #ceil | #trunc | #floor]=?,
unit
) => t = ""
}
@send external round: (t, ~to: roundTo) => t = "round"
@send external roundWithOptions: (t, RoundOptions.t) => t = "round"
type roundOptions
@obj
external roundOptions: (
~smallestUnit: roundTo,
~roundingIncrement: int=?,
~roundingMode: [#halfExpand | #ceil | #trunc | #floor]=?,
unit
) => roundOptions = ""
@send external round: (t, ~to: roundTo) => t = "round"
@send external roundWithOptions: (t, roundOptions) => t = "round"

And so on for the other submodules you've added in this PR. In this style using it becomes value->Instant.roundWithOptions(Instant.roundOptions(~smallestUnit=#hour, ())).

Although to be fair by the time we get to v2 we might have ReScript 10 available, and if the structural typing change in master works as well as we hope the entire @obj system will be replaced.


@new
external make: (
~isoYear: int,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The from methods don't prefix the keys with iso. So I think even though the spec describes them as isoYear etc we should stay consistent and not prefix the parameters.


@scope("Temporal.ZonedDateTime") @val external fromString: string => t = "from"
@scope("Temporal.ZonedDateTime") @val
external fromStringWithOptions: (string, FromOptions.t) => t = "from"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record types are best when all properties are required, imo. They have the advantage of looking like objects when calling the function; the @obj system was created a long time ago and is now only used as a bit of a hack to omit unspecified properties instead of setting them to undefined.

And yes now that we're getting more contributors a style guide is probably worth starting. It might make a nice binding cheat-sheet too.

@tom-sherman tom-sherman mentioned this pull request Jan 1, 2022
@TheSpyder TheSpyder force-pushed the main branch 5 times, most recently from fe0f2e9 to 7f2cf0a Compare May 22, 2024 12:41
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

2 participants