Closed Bug 419433 Opened 15 years ago Closed 15 years ago

Remove use of ISO date helpers from Microformats

Categories

(Toolkit Graveyard :: Microformats, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file)

When this change was originally made to my code (without any review or approval by me, I might add), I thought it would work, but I have since discovered it broke my microformats code.

1. It removed an API that is used by implementors (Microformats.dataFromISO8601), 
2. ISO dates can have a date but no time. When they are converted to JS objects, those objects have a time of midnight and you can never find out that they didn't have a time associated with them. My implementation uses an internal boolean (time) to indicate if the ISO date that was passed in has a time or not. This is important when converting back to an ISO date.

So this bug puts my code back (with a test to make sure this doesn't happen again)
Attachment #305527 - Flags: review?(sayrer)
Attachment #305527 - Flags: review?(sayrer) → review+
Blocks: 398177
Attachment #305527 - Flags: approval1.9?
Wouldn't it be better to change the ISO8601DateUtils code to cover case #2 instead, so other users get the functionality as well?
(In reply to comment #2)
> Wouldn't it be better to change the ISO8601DateUtils code to cover case #2
> instead, so other users get the functionality as well?

That would be difficult simply because JS Date objects don't support Date only.

So for instance, how would doing a toLocaleString on a ISO date that was converted to JS know to only display the date?
Don't these same questions apply to the code you're adding back?
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

a=beltzner for 1.9
Attachment #305527 - Flags: approval1.9? → approval1.9+
(In reply to comment #4)
> Don't these same questions apply to the code you're adding back?
> 

Since I'm parsing the date, I can know if it it has a time or not, so I set a boolean on the JS object to indicate that it is a date only.

Then when I convert the JS object back to an ISO date, I don't add the time.

ISO code could in theory do something similar.
(In reply to comment #6)
> ISO code could in theory do something similar.

Exactly, so it seems to me that changing ISO8601DateUtils to do what you want would be better, since it helps more than just microsummaries users.
Fix checked in.

ISO dates with just the date are not used by many other things, but if I have time I'll fix the core ISO date code.

This needs to go in to make FF 3.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
This completely landed without approval. http://tinderbox.mozilla.org/Firefox/ clearly says "The tree is frozen for beta4. Only checkins with approval1.9b4+ are permitted." You either need to get post-approval, or you should back out now.
Sorry, I thought approval 1.9 was enough.
Reopened since I backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #305527 - Flags: approval1.9b4?
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

a1.9b4=beltzner
Attachment #305527 - Flags: approval1.9b4? → approval1.9b4+
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Actually, can we hold off on landing this? We need to hold the tree for a bit to try out some build flag changes. I'll remark as approved when you're free to land.
Attachment #305527 - Flags: approval1.9b4+ → approval1.9b4?
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

OK, good to go again. Please land for beta4.
Attachment #305527 - Flags: approval1.9b4? → approval1.9b4+
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Sadly, this missed the beta 4 checkin cutoff. Please land after we open for post-b4 checkins.
Attachment #305527 - Flags: approval1.9b4+ → approval1.9b4-
Actually I got it in. Forgot to close the bug.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
FWIW, here's the bonsai link for the checkin that stuck:
  http://tinyurl.com/2ooa74
Target Milestone: --- → mozilla1.9beta4
Comment on attachment 305527 [details] [diff] [review]
Put my code back and add a test

Apparently this did land before the cut off. Putting the flag to rights for some reason which honestly eludes me.
Attachment #305527 - Flags: approval1.9b4- → approval1.9b4+
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.