767588 - GCLI number type should allow restriction to integers

Status: RESOLVED FIXED

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

Comment 1

[Abhishek Potnis [:avp]]

Hi Joe,

I would like to work on this bug. Please help me on getting started with it.

Thanks.

Comment 2

[Joe Walker [:jwalker]]

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

The number type is in lib/gcli/types/basic.js.

It may be as simple as adding a property, and I think the default should be int because there are far more cases where you actually want an integer than a float. so perhaps the syntax should be:

type: 'number'                              <- gets an int
type: { name: 'number', allowFloat: true }  <- gets a float

So we'd need to add to the NumberType constructor:

  this._allowFloat = typeSpec.allowFloat;

And then alter NumberType.prototype.parse to check for (this._allowFloat === true) before complaining if the input string contains a '.' (see the other examples in that file)

Hope that helps.

Comment 3

[Joe Walker [:jwalker]]

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

Comment 4

[Adam Goforth]

Attached is a proposed patch for this bug.  I don't know if you prefer pull requests, so I sent one of those as well:
https://github.com/joewalker/gcli/pull/7

There were a couple of unspecified details:

* What happens if a number is created like this?

    type: { name: 'number', allowFloat: false, min: -5.5, max: 5.5, step: 1.5 }

  In my patch, the constructor rounds the values to ints so that increment and decrement don't create invalid numbers.

* What indication should the user have about what kind of number is required?

  In my patch, the number's name is set to 'integer' when appropriate, so either 'integer' or 'number' will show up in the help.

Comment 5

[Adam Goforth]

Attachment: First proposed patch

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 714815 [details] [diff] [review]
First proposed patch

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

This is excellent, thanks Adam.

The bad news is that we're quite close to landing a significant update to GCLI to allow async types. This doesn't affect your changes to basic.js at all, but it does affect the tests because the test system has to take asynchronous results into account. Please could you take a look at the asynctypes-685526 branch?

The place to start looking is gclitest/helpers.js: helpers.audit() and what testCli, testCompletion and testKeyboard look like now.
Thanks!

::: lib/gcli/nls/strings.js
@@ +80,4 @@
>      // When the command line is passed a number, but the number is lower than
>      // the smallest allowed number, this error message is displayed.
>      typesNumberMin: '%1$S is smaller than minimum allowed: %2$S.',
> +    

This is a minor nit which I would generally remove myself on commit, but since there are some updates to do, perhaps you could remove the trailing space here?

::: lib/gcli/types/basic.js
@@ +86,4 @@
>      this._min = typeSpec.min;
>      this._max = typeSpec.max;
>      this._step = typeSpec.step || 1;
> +    this._allowFloat = typeSpec.allowFloat || false;

Or
  this._allowFloat = !!typeSpec.allowFloat;
to coerce to a boolean, with a default of false?

Which would mean you could drop the 'this._allowFloat = false;' below too.

@@ +95,5 @@
> +      if (typeof this._max === 'number') {
> +        this._max = Math.floor(this._max);
> +      }
> +      this._step = Math.round(this._step);
> +      this.name = 'integer';

I think that trying to change the name here is probably a mistake for a couple of reasons:
* When registered the type had the name 'number', and is remembered that way by the type system
* JS doesn't really have an int type, so all we're really doing is making a promise that we won't use the fractional part. Having a number type with an allowFloat property feels like the best way to fit in here.

Comment 7

[Joe Walker [:jwalker]]

(In reply to Adam Goforth from comment #4)
> Attached is a proposed patch for this bug.  I don't know if you prefer pull
> requests, so I sent one of those as well:
> https://github.com/joewalker/gcli/pull/7
> 
> There were a couple of unspecified details:
> 
> * What happens if a number is created like this?
> 
>     type: { name: 'number', allowFloat: false, min: -5.5, max: 5.5, step:
> 1.5 }
> 
>   In my patch, the constructor rounds the values to ints so that increment
> and decrement don't create invalid numbers.

On reflection, I think we should probably throw for this case, because the type proposed is internally inconsistent.

> * What indication should the user have about what kind of number is required?
> 
>   In my patch, the number's name is set to 'integer' when appropriate, so
> either 'integer' or 'number' will show up in the help.

See the comment above.
I hope that for the most part the answer will be obvious from the context:

>> sleep 1.5

(Clearly not an error, user would probably guess to type a fraction when they wanted a fractional delay)

>> edit foo.txt --line 10

(User would probably guess that this was an integer, and if they typed "--line 2.5" they would get an error message saying '2.5 must be an integer' to clear things up)

Comment 8

[Adam Goforth]

Attachment: Implement changes requested in review.

Joe,

This patch implements the changes you requested, except for the async types test changes.

> The bad news is that we're quite close to landing a significant update
> to GCLI to allow async types. This doesn't affect your changes to
> basic.js at all, but it does affect the tests because the test system
> has to take asynchronous results into account. Please could you take
> a look at the asynctypes-685526 branch?

How would you like to handle this?  I could wait until the async types changes land, then merge them into my branch and update my tests.  Alternatively, I could change my patch to apply onto the current asynctypes-685526 branch.  Do you have a preference?

Comment 9

[Adam Goforth]

Attachment: Full patch with post-review changes.

Sorry, the last patch only included the most recent changes, not all of the changes for the feature.  I believe this one is what's expected.

Comment 10

[Joe Walker [:jwalker]]

(In reply to Adam Goforth from comment #8)
> How would you like to handle this?  I could wait until the async types
> changes land, then merge them into my branch and update my tests. 
> Alternatively, I could change my patch to apply onto the current
> asynctypes-685526 branch.  Do you have a preference?

I'm having trouble landing asynctypes-685526 because it touches quite a few parts, so lets first get your change in, merged into master and then we can merge from master into the asynctypes-685526 branch and do the updates at that time. That way I don't hold you up any more.

I'll run this past try (which should be automatic since it passes travis) and then I'll push.

Thanks!

Comment 11

[Joe Walker [:jwalker]]

Attachment: How the patch affects central

Comment 12

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=9f33fb88d95f

Comment 13

[Joe Walker [:jwalker]]

Attachment: upload 4

The additional mock command broke a 'not found' test in an otherwise unrelated test. This update fixes the other test.
Prev try includes these changes.

Comment 14

[Joe Walker [:jwalker]]

https://hg.mozilla.org/integration/fx-team/rev/a8a47ff4771e

Many thanks Adam.

Comment 15

[Rob Campbell [:rc] (:robcee)]

good first patch, Adam!

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a8a47ff4771e