1230549 - Storage should pass eslint

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Let's fix Storage code to pass mach eslint

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: MozReview Request: Bug 1230549 - Make Storage pass basic eslint. r?yoric

Bug 1230549 - Make Storage pass basic eslint. r?yoric

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: MozReview Request: Bug 1230549 - Storage should pass more eslint rules. r?yoric

Bug 1230549 - Storage should pass more eslint rules. r?yoric

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8695916 [details]
MozReview Request: Bug 1230549 - Make Storage pass basic eslint. r?yoric

https://reviewboard.mozilla.org/r/27215/#review24643

Looks good to me. I haven't checked against the rules of `.eslintrc`, please tell me if you believe that I should.

::: storage/test/unit/test_storage_connection.js:150
(Diff revision 1)
>    try {

Could you file a mentored bug to turn this into a use of `Assert.throws`?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8695917 [details]
MozReview Request: Bug 1230549 - Storage should pass more eslint rules. r?yoric

https://reviewboard.mozilla.org/r/27217/#review24651

::: storage/test/unit/head_storage.js:58
(Diff revision 1)
> -    try { dbFile.remove(false); } catch(e) { /* stupid windows box */ }
> +    try { dbFile.remove(false); } catch (e) { /* stupid windows box */ }

Whitespace change? Is this really necessary?

Here and a few others below.

::: storage/test/unit/test_connection_executeAsync.js:45
(Diff revision 1)
> -    {
> +      dump("handleResult(" + aResultSet + ")\n");

Could you take the opportunity to file a mentored bug for replacing `dump` with `do_print` in this file?

I can mentor.

::: storage/test/unit/test_statement_executeAsync.js:908
(Diff revision 1)
> -var tests =
> +var tests = [

Can you file a followup bug to convert this to `add_test`/`add_task`?

::: storage/test/unit/test_storage_aggregates.js:82
(Diff revision 1)
> -  while(stmt.executeStep());
> +  while (stmt.executeStep());

Could you take the opportunity to add `{ /* do nothing */ }`?

::: storage/test/unit/test_storage_connection.js:251
(Diff revision 1)
>      if (temp.exists()) try {

Could you take the opportunity to add braces here?

::: storage/test/unit/test_storage_function.js:65
(Diff revision 1)
> -  while(stmt.executeStep());
> +  while (stmt.executeStep());

Here, too, `{ /* do nothing */ }`.

::: storage/test/unit/test_storage_progresshandler.js:62
(Diff revision 1)
> -  while(stmt.executeStep());
> +  while (stmt.executeStep());

Here, too, `{ /* do nothing */ }`.

::: storage/test/unit/test_storage_progresshandler.js:78
(Diff revision 1)
> -    while(stmt.executeStep());
> +    while (stmt.executeStep());

Same here.

::: storage/test/unit/test_storage_value_array.js:205
(Diff revision 1)
> -  for (var i = 0; i < tests.length; i++)
> +  for (var i = 0; i < tests.length; i++) {

Looks like a good candidate for `add_test`. Followup (mentored) bug?

::: storage/test/unit/test_unicode.js:98
(Diff revision 1)
> -  for (var i = 0; i < tests.length; i++)
> +  for (var i = 0; i < tests.length; i++) {

Here, too, `add_test`?

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://reviewboard.mozilla.org/r/27217/#review24651

> Whitespace change? Is this really necessary?
> 
> Here and a few others below.

it's part of the coding style and we have an eslint rule (that likely will be further discussed, in the meanwhile I'll just stick to the coding style guide)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0914d89dc7b9
https://hg.mozilla.org/integration/fx-team/rev/dc9d0cfb6129

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0914d89dc7b9
https://hg.mozilla.org/mozilla-central/rev/dc9d0cfb6129