Closed Bug 469477 Opened 11 years ago Closed 11 years ago

Move fromRFC3339 from calGoogleUtils.js to calProviderUtils.js

Categories

(Calendar :: Provider: GData, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

(Whiteboard: [gdata-next])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20081201 Lightning/1.0pre Thunderbird/3.0b1

calGoogleUtils.js contains a function fromRFC3339 which can parse a RFC 3339 date string into a TimeDate object.  This would be useful for other providers to use besides GData.  Moving it to calProviderUtils.js would allow other providers access.

Reproducible: Always
Just
Attached patch Patch to Move functions (obsolete) β€” β€” Splinter Review
Patch to move fromRFC3339 and toRFC3339 from calGoogleUtils.js to calProviderUtils.js
Attachment #352848 - Flags: review?(philipp)
Comment on attachment 352848 [details] [diff] [review]
Patch to Move functions

Codewise this looks good, checking in Monday or Tuesday to allow for comments.

r=philipp
Attachment #352848 - Flags: review?(philipp) → review+
Looking foward to your unit tests. Go ahead and attach it to this bug when you are done.
Assignee: nobody → DarkJedi613
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: in-testsuite?
Whiteboard: [gdata-next]
What further provider needs these functions? Please mind that every function added to calProviderUtils.js is nailed and cannot be removed later on without potentially breaking 3rd party code.
Finally, I suggest we introduce a js module file for provider utils, instead of adding more stuff to calProviderUtils.js.
It actually came up when discussing how to produce a testsuite for the fromRFC3339 function.  It didn't seem there was an elegant way to have it run on build.  Since RFC 3339 dates are quite common on the internet it seemed that it would be useful to other providers moving it to calProviderUtils would allow this in addition to allowing unit tests to be ran on the functions.

I know the following use the fromRFC3339:
GData
hCalendar (https://addons.mozilla.org/en-US/thunderbird/addon/6702)
Remember the Milk (https://addons.mozilla.org/en-US/sunbird/addon/7125)
as well as a provider I've been working on
They're just copied from the GData extension (and renamed in the case of RtM provider)

I suppose it could also be part of the DateTime object, but that wouldn't address the issue of it being nailed. What benefits come from using a JS module as opposed to calProviderUtils.js?
Updated patch to use calProviderUtils.jsm

A module is the new way of including functions from other files, see https://developer.mozilla.org/En/JavaScript_modules

There is also a date utils module in core code, but it uses jsDate instead of calIDateTime.
Attachment #352848 - Attachment is obsolete: true
Attachment #357558 - Flags: review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/abb3fe7271d8>

(and unbreak things I did wrong in rev d06f1cb6732c)

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Attached patch Testsuite for fromRFC3339 and toRFC3339 (obsolete) β€” β€” Splinter Review
Attachment #359506 - Attachment is patch: true
Attachment #359506 - Attachment mime type: application/octet-stream → text/plain
Attachment #359506 - Flags: review?(philipp)
Attachment #359506 - Flags: review?(philipp) → review-
Comment on attachment 359506 [details] [diff] [review]
Testsuite for fromRFC3339 and toRFC3339

>+	
>+	testRFC3339_();
Indentation is not quite correct here since you use tabs. Please expand tabs to spaces.

>+		var dateTime = cal.fromRFC3339(aRFC3339Date, aTimezone);
Please use let instead of var everywhere in your patch.


>+		do_check_eq(dateTime, aResult);
>+		var RFC3339 = cal.toRFC3339(dateTime);
JS convention is camelCase, please use rfc3339Date or such.

>+		// In theory this should be:
>+		// do_check_eq(RFC3339, aRFC3339Date);
Why only in theory? Please explain in the code comment.

>+	 * All tests are done under the default timezone and UTC (although both
>+	 * should give the same time)
Good start, although it would be great to also test a specific timezone that has DST boundaries and see if the function behaves well when crossing the boundary. Which timezone you use doesn't maktter.

>+	// This represents March 14, 2006 in the default timezone
        testRFC3339_("2006-03-14",
                     timezone,
                     "2006/03/14 00:00:00 " + timezone + " isDate=1",
                     "2006-03-14");
We tend to wrap parameters a bit differently, one parameter per line if the line would otherwise exceed 80 characters. The above is an example of how that could look like.


>+	testRFC3339_("2004-10-14T03:00:23-06:00", timezone, "2004/10/14 03:00:23 America/Belize isDate=0",
>+				 "2004-10-14T03:00:23-06:00");
You said you use the default timezone. Please note that the default timezone may not be America/Belize for everyone. Unless you are already doing so, please use it explicitly (on which occasion you could test the DST boundaries).

I'm a bit worried that using calIDateTime::toString() might not be very failsafe. You could consider testing the memers directly. you could pass the expected date like:

[2004,10,14,3,0,23,timezone] or [2004,10,14,-1,-1,-1,timezone] for allday dates.

then some magic like


let propmap = { year: expected[0],
                month: expected[1],
                ...
              };

for (let prop in propmap) {
    do_check_eq(theDate[prop], propmap[prop]);
}

I won't require this right now though, we can fix it if it fails.

Overall the patch looks very nice, only setting r- so you can consider the comments above. Please upload new patch and rerequest review.
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
It only took a bit over 5 years, but I got back to this. Thanks for the review. :)
Attachment #359506 - Attachment is obsolete: true
Attachment #8416478 - Flags: review?(philipp)
Comment on attachment 8416478 [details] [diff] [review]
Testsuite for fromRFC3339 and toRFC3339 v2

Review of attachment 8416478 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=philipp with this minor nit:

::: calendar/test/unit/test_rfc3339_parser.js
@@ +59,5 @@
> +    let timezone = getTz("/mozilla.org/20071231_1/America/New_York");
> +    let utc = cal.UTC();
> +    // Timezones used in tests.
> +    belize = getTz("/mozilla.org/20071231_1/America/Belize");
> +    dawson = getTz("/mozilla.org/20071231_1/America/Dawson");

I think you can drop the prefix on these, i.e just America/New_York
Attachment #8416478 - Flags: review?(philipp) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> Looks good, r=philipp with this minor nit:

> I think you can drop the prefix on these, i.e just America/New_York
Thought I tested that, but you're right: it works without those!

Thanks for the review. I'll push (with the nit and commit message fixed) once the tree is back open.
https://hg.mozilla.org/comm-central/rev/c9552299a4e7
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.