1483962 - Move compile-time dependent Date helpers into a class

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[André Bargull [:anba]]

See bug 1346211, comment #19:
> It might be nice, in a followup, to put all these functions as statics inside a
> class, so that the numerous helper functions could be private and clearly 
> *only* used as helper function inside the three public static signatures 
> actually used by the rest of the file.  (And there could be standalone helper 
> functions to call the class-statics, so as to avoid touching the world to 
> rename things.)

Comment 1

[Thi Huynh]

Hi, I've created a patch for this on my local machine but cannot push to phabricator. The arc command complained that there is no reviewer name anba. How should I send this patch to you?

Comment 2

[André Bargull [:anba]]

I haven't yet created a Phabricator account, but even with an account there'll probably be permission issues when doing a review. Waldo actually requested to move the functions into a helper class, so you probably want to ask him for review anyway.

Comment 3

[Thi Huynh]

Got it, thank you :) .

Comment 4

[Thi Huynh]

Attachment: Bug 1483962 - Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden

Comment 5

[Jan de Mooij [:jandem]]

Waldo, can we r+/land this now?

Thi, feel free to needinfo people like this if you need help :)

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 9002806 [details]
Bug 1483962 - Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden

Jeff Walden [:Waldo] has approved the revision.

Comment 7

[Jeff Walden [:Waldo]]

*mutters about phabricator updates not showing up in bugs at all*

Comment 8

[Thi Huynh]

With the current patch I think we don't need to move the DateTimeHelper function definitions to inside the class because we cannot misuse the helper functions anymore. Do you think we have anything else to do before getting this landed, Waldo?

Comment 9

[Jeff Walden [:Waldo]]

I may move them inside the function body myself at some point, but it's not critical it happen now.  And certainly not if phabricator doesn't have a way to consider multiple patches all at once.

I was assuming the point of this phabricator thing was that it would just basically land once stuff had reviews on its own.  Maybe I'm wrong about that?  Will look into this today sometime and figure it out.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] from comment #9)
> I was assuming the point of this phabricator thing was that it would just
> basically land once stuff had reviews on its own.  Maybe I'm wrong about
> that?  Will look into this today sometime and figure it out.

You can click "View in Lando" (on the right side) and there you can press the big green button ✅.

Comment 11

[Jeff Walden [:Waldo]]

...or at least I can if I can get L3 commit access applied to the LDAP for jwalden at moz, since jwalden at mit is the one that actually has L3 access.  :-|  Bah.  Will figure this out somehow.

Comment 12

[Jeff Walden [:Waldo]]

I still need to figure out LDAP for this, but really I should probably cut my losses and make someone else pushbutton this in, at this point.  :-|

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2206a13d6d3
Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r=jwalden

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e2206a13d6d3