GCLI needs a date type

RESOLVED FIXED in Future

Status

P3
normal
RESOLVED FIXED
7 years ago
6 months ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

Trunk
Future
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

For the cookie command for example.
Work for this has begun in https://github.com/joewalker/gcli/tree/date-773271
Whiteboard: [good first bug][mentor=jwalker]

Comment 2

7 years ago
Hi Joe Walker,
I am interested in working on this bug. Could you please guide me on getting started with it.
Thanks.
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

7 years ago
Hi gothu000 and Joe, are you still working on this bug ? If not, can i take over it ? Thanks a lot!
(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!
No longer blocks: 784790
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
Posted patch First draft for a date type (obsolete) — Splinter Review
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.
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.
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.
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
(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
Posted patch v2 (obsolete) — Splinter Review
Attachment #694846 - Attachment is obsolete: true
Posted patch v3 (obsolete) — Splinter Review
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
Attachment #750433 - Attachment is obsolete: true
Attachment #751016 - Flags: review?(fayearthur)
Posted patch v4 (obsolete) — Splinter Review
Fixes file missing from previous patch.
Assignee: nobody → jwalker
Attachment #751016 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #751016 - Flags: review?(fayearthur)
Attachment #751386 - Flags: review?(fayearthur)
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?
Attachment #751386 - Flags: review?(fayearthur) → review+
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
Posted patch v5Splinter Review
Attachment #751386 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/3b90dbff5809
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

9 months ago
Product: Firefox → DevTools

Updated

6 months ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.