search

hhvm / hsl-experimental

Experimental features for the Hack Standard Library
MIT License
23 stars 10 forks source link

HSL DateTime: Zoned and Unzoned #184

Closed jjergus closed 3 years ago

jjergus commented 3 years ago

This is the last remaining part from https://github.com/jjergus/hsl-experimental/tree/dt4/src/datetime

I haven't added any tests yet (so I may discover and fix some bugs while writing them), but wanted to get this out so I can start getting feedback while I work on the tests.

jjergus commented 3 years ago

I added a new commit that adds DateTime to all class names (ZonedDateTime, UnzonedDateTime, DateTimeBuilder) so we can see if it feels better that way or not.

I don't have a strong opinion (both versions seem fine to me), but I don't completely buy your argument that the class names must be unambiguous when used without their namespace. It's a pretty common pattern across HSL that the namespace is an important part of class/function names (Dict\filter vs Vec\filter, TCP\Socket vs Unix\Socket, etc.)

fredemmott commented 3 years ago

My 2c: it should just be DateTime\Unzoned

It's never been explicit, but the assumption has always been that the non-_Private parts of the HSL will only be aliased via use namespace. Ján's Vec\filter() and Dict\filter() examples are illustrative, as is the fact that we only support configurable namespace aliases in .hhconfig/.hhvmconfig.hdf, not function imports.

Assuming that continues, this is going to be much more verbose (and inconsistent with the rest of the HSL) for the common case, and as is available for the rarer cases.

Perhaps it's worth bringing this up in the #hsl slack channel for broader input?

jjergus commented 3 years ago

non-_Private parts of the HSL will only be aliased via use namespace

I'm not sure if this is a good restriction that we should be aiming to preserve. I kind of violated it already with HH\Lib\Experimental\Time which can only naturally be aliased as use type HH\Lib\Experimental\Time. I could move that to the DateTime namespace, though DateTime\Time doesn't look great either.

Overall, I think it's reasonable to have different names designed to be most naturally aliased in different ways. xhp-lib has a lot of interesting examples, e.g. HTML elements (many would probably choose to use type those, but something like use namespace Facebook\XHP\HTML as h is also a good option, or even use namespace Facebook\XHP\HTML without an alias).

jjergus commented 3 years ago

OK I reverted the renaming commit, since it seems both Fred and I prefer the shorter names (we can of course revert it back in the future if we change our minds).

fredemmott commented 3 years ago

I'm not sure if this is a good restriction that we should be aiming to preserve. I kind of violated it already with HH\Lib\Experimental\Time which can only naturally be aliased as use type HH\Lib\Experimental\Time. I could move that to the DateTime namespace, though DateTime\Time doesn't look great either.

Yeah, we also have HH\Lib\Ref as an exception; on the one hand, I think it's a good general rule and it's fine to have well-reasoned exceptions - but, this also does interfere with the use of namespace aliasing as a shortcut for the HSL.

For now, that's basically mono-repo only, but I hope we'll eventually be able to use typechecker alias maps in subdirs like we can for the runtime with .hhvmconfig.hdf

facebook-github-bot commented 3 years ago

Hi @jjergus!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

jjergus commented 3 years ago

(I'm still working on the remaining test cases, I'll send out a separate PR when they're ready.)