773271 - GCLI needs a date type

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P3)

Description

[Joe Walker [:jwalker]]

For the cookie command for example.

Comment 1

[Joe Walker [:jwalker]]

Work for this has begun in https://github.com/joewalker/gcli/tree/date-773271

Comment 2

[gothu000]

Hi Joe Walker,
I am interested in working on this bug. Could you please guide me on getting started with it.
Thanks.

Comment 3

[Joe Walker [:jwalker]]

See https://developer.mozilla.org/en-US/docs/Tools/GCLI, particularly 'contributing the the command line'.

The date type is in lib/gcli/types/date.js.

There are some notes (particularly TODO comments) in this file.

Comment 4

[tienthanh8490]

Hi gothu000 and Joe, are you still working on this bug ? If not, can i take over it ? Thanks a lot!

Comment 5

[Joe Walker [:jwalker]]

(In reply to tienthanh8490 from comment #4)
> Hi gothu000 and Joe, are you still working on this bug ? If not, can i take
> over it ? Thanks a lot!

Please do!

Comment 6

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 7

[Quentin Pradet [:quentinp]]

Attachment: First draft for a date type

This is a working prototype for a GCLI date type - it works in a demo command named "gcli onedate". It supports the increment/decrement/parse/stringify operations, but not min/max: I don't think they make sense for a date. Increments steps are specifiable, and the unit is currently days. For example, with a step of 5, 'Tue Jan 15 2013 15:46:29 GMT+0100 (CET)' becomes 'Sun Jan 20 2013 15:46:29 GMT+0100 (CET)'.

Issues:
 * testDate.js has not been updated (I don't know/remember how to launch it).
 * There is no integration with the cookie command yet (I don't know how to use the corresponding jsm file).
 * MDN says that n should be between 1 and 31 for Date.setDate(n), but it seems to work with any number on Firefox and Chrome, and I "abuse" this behavior.

The current diff applies on a merge between master and date-773271. I'm new to git, so this may not be the appropriate thing to do. I can provide something else if needed. I'm sure there's a lot more things to do but I need more guidance to go forward. Oh, and I'll be away for one month, so anyone can work on this if they feel like it.

Comment 8

[Joe Walker [:jwalker]]

Thanks Quentin. Clearly I'm a bit slow reviewing this with the holidays, but I'll get to it fairly soon.

I'd like to get a fairly clear sign-off that you're OK with me adding this code to GCLI. A number of big organizations use GCLI, so they need the IP to be clear. Please could you do something like this:

  git checkout -b signoffleak joewalker/leak-795988
  git commit --amend
    (in the editor add a sign-off line. See docs [1] or example [2])
  git push origin signoffleak

Then in Github do a pull request, and I'll sort the rest out.

[1] https://github.com/joewalker/gcli/blob/master/docs/developing-gcli.md
[2] https://github.com/joewalker/gcli/commit/2c157a93bea98caca25593fa0975f2cba0c894e9

Thanks.

Comment 9

[Joe Walker [:jwalker]]

Just been reviewing the patch, thanks. I think max/min are quite useful with dates, it's not going to be uncommon for someone to want to say "not before now" (i.e. at some stage in the future) and with things like cookies there is an upper limit on how far away the expiry date can be set.

There is documentation on how to run tests in https://github.com/joewalker/gcli/blob/master/docs/developing-gcli.md.

Comment 10

[Quentin Pradet [:quentinp]]

For the record, I've answered to comments and posted a pull request on GitHub [0] which still needs a few improvements. This bug is also blocked by bug 865631.

[0] https://github.com/mozilla/gcli/pull/63

Comment 11

[Joe Walker [:jwalker]]

(In reply to Quentin Pradet from comment #10)
> For the record, I've answered to comments and posted a pull request on
> GitHub [0] which still needs a few improvements. This bug is also blocked by
> bug 865631.
> 
> [0] https://github.com/mozilla/gcli/pull/63

Thanks for the reminder. I'll take a look

Comment 12

[Joe Walker [:jwalker]]

Attachment: v2

Comment 13

[Joe Walker [:jwalker]]

Attachment: v3

This patch was written by a number of people and has undergone a fairly in-depth review at https://github.com/joewalker/gcli/pull/10 and https://github.com/mozilla/gcli/pull/63

Comment 14

[Joe Walker [:jwalker]]

Attachment: v4

Fixes file missing from previous patch.

Comment 15

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=2b5ccd0f6279

Comment 16

[Heather Arthur [:harth]]

Comment on attachment 751386 [details] [diff] [review]
v4

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

Looks good, thanks Quentin.

::: toolkit/devtools/gcli/gcli.jsm
@@ +4004,5 @@
> +
> +  var str = value.toISOString();
> +
> +  if (value.getHours() === 0 && value.getMinutes() === 0 &&
> +      value.getSeconds() === 0 && value.getMilliseconds() === 0) {

Maybe comment about this case and why its stripping the T?

Comment 17

[Quentin Pradet [:quentinp]]

Thanks for the review!

Most of the work here is from Joe. The idea was to remove useless information for simple dates like `2013/5/21` [0]. Unfortunately, ISO 8601 (`toISOString()`) uses UTC (aka. GMT), which causes a number of subtle issues [1] [2].

The fix chosen [3] (pushed on github, but not updated on bugzilla yet) is to stringify dates with a toISOString()-like formatting whith local time (`getHours()` instead of `getUTCHours()`).

[0] https://github.com/joewalker/gcli/commit/6f51f881060e57184504650c3d89cf5d8a91c511
[1] https://github.com/joewalker/gcli/pull/10#commitcomment-3229403
[2] https://github.com/joewalker/gcli/pull/10#commitcomment-3235544
[3] https://github.com/joewalker/gcli/commit/34e908f654e46021776d814f8f22252406ed4a98

Comment 18

[Joe Walker [:jwalker]]

Attachment: v5

Comment 19

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=6cf5ac92530b
https://hg.mozilla.org/integration/fx-team/rev/3b90dbff5809

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b90dbff5809