Closed Bug 1483962 Opened 2 years ago Closed 2 years ago

Move compile-time dependent Date helpers into a class

Categories

(Core :: JavaScript: Standard Library, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: anba, Assigned: so61pi.re)

Details

Attachments

(1 file)

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.)
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?
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.
Got it, thank you :) .
Assignee: nobody → so61pi.re
Status: NEW → ASSIGNED
Attachment #9002806 - Attachment description: Bug 1483962 - Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden → Bug 1483962 - Part 1: Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden
Waldo, can we r+/land this now?

Thi, feel free to needinfo people like this if you need help :)
Flags: needinfo?(jwalden+bmo)
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.
Attachment #9002806 - Flags: review+
*mutters about phabricator updates not showing up in bugs at all*
Flags: needinfo?(jwalden+bmo)
Attachment #9002806 - Attachment description: Bug 1483962 - Part 1: Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden → Bug 1483962 - Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r?jwalden
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?
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
(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 ✅.
...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.
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.  :-|
Keywords: checkin-needed
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2206a13d6d3
Move helpers of LocalTime UTC TimeZoneComment functions to DateTimeHelper. r=jwalden
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2206a13d6d3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.