Closed Bug 1230549 Opened 4 years ago Closed 4 years ago

Storage should pass eslint

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(2 files)

Let's fix Storage code to pass mach eslint
Bug 1230549 - Make Storage pass basic eslint. r?yoric
Attachment #8695916 - Flags: review?(dteller)
Bug 1230549 - Storage should pass more eslint rules. r?yoric
Attachment #8695917 - Flags: review?(dteller)
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`?
Attachment #8695916 - Flags: review?(dteller) → review+
Attachment #8695917 - Flags: review?(dteller) → review+
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`?
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)
https://hg.mozilla.org/mozilla-central/rev/0914d89dc7b9
https://hg.mozilla.org/mozilla-central/rev/dc9d0cfb6129
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.