Closed Bug 1301287 Opened 5 years ago Closed 5 years ago

Assorted code style fixes (no-array-constructor, brace-style, comments, semi, space-infix-ops, no-new-object, no-array-constructor)

Categories

(Toolkit :: Password Manager, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: MattN, Assigned: MattN)

Details

Attachments

(6 files)

Assorted code style fixes and enabling of rules with eslint to enforce consistency in the future.
Flags: qe-verify-
If the reason to keep the "ids" exception in the "no-unused-vars" rule was only to preserve blame, you can take this occasion to remove the exception :-)
Most of these style changes are touching trivial lines whereas that isn't the case for changing `ids`. Not changing blame wasn't the only reason for the exception but it was hard for me to express. Partly that not using all of the destructured array arguments feels a bit weird especially when all(?) callers ignore it and partly that I want to remove all references to `id` in storage-json since I don't know why we kept that property in the first place (I've been meaning to ask you) since it's not exposed outside storage (though maybe one can search on it?) and we have a guid.
(In reply to Matthew N. [:MattN] from comment #8)
> the exception but it was hard for me to express. Partly that not using all
> of the destructured array arguments feels a bit weird especially when all(?)
> callers ignore it

So I checked this when reviewing the other patch and unfortunately it seems to be used in one instance.

This was why I was proposing renaming ids to _ids in the other cases, to stay consistent with the return pattern. But I think a general cleanup of that function sounds like the proper way to handle it, so I wouldn't mind keeping the exception if we want to solve this eventually.
(In reply to Matthew N. [:MattN] from comment #8)
> I want to remove all references to `id` in
> storage-json since I don't know why we kept that property in the first place

If I remember correctly, when I looked into that I determined the property was still needed to reduce risk around add-on compatibility and migration. The situation may have changed, and we can break compatibility now that the new back-end has stabilized.
Comment on attachment 8789203 [details]
Bug 1301287 - Password manager: enable eslint brace-style rule on shipping code.

https://reviewboard.mozilla.org/r/77490/#review75898

::: toolkit/components/passwordmgr/storage-mozStorage.js:30
(Diff revision 1)
>  
>    this._hasTransaction = false;
>    try {
>      this._db.beginTransaction();
>      this._hasTransaction = true;
> +  } catch (e) { /* om nom nom exceptions */ }

sigh
Comment on attachment 8789203 [details]
Bug 1301287 - Password manager: enable eslint brace-style rule on shipping code.

https://reviewboard.mozilla.org/r/77490/#review75900

So the changes look good but I wonder why we disable this for tests, would there be too many changes to be made?
Attachment #8789203 - Flags: review?(jhofmann) → review+
Comment on attachment 8789204 [details]
Bug 1301287 - Remove useless method comments and convert others to JSDoc style.

https://reviewboard.mozilla.org/r/77492/#review75902
Attachment #8789204 - Flags: review?(jhofmann) → review+
Comment on attachment 8789205 [details]
Bug 1301287 - Password manager: enable eslint semi rule.

https://reviewboard.mozilla.org/r/77494/#review75904
Attachment #8789205 - Flags: review?(jhofmann) → review+
Comment on attachment 8789206 [details]
Bug 1301287 - Password manager: enable eslint space-infix-ops rule.

https://reviewboard.mozilla.org/r/77496/#review75906
Attachment #8789206 - Flags: review?(jhofmann) → review+
Comment on attachment 8789207 [details]
Bug 1301287 - Password manager: enable eslint no-new-object rule.

https://reviewboard.mozilla.org/r/77498/#review75908
Attachment #8789207 - Flags: review?(jhofmann) → review+
Comment on attachment 8789208 [details]
Bug 1301287 - Password manager: enable eslint no-array-constructor rule.

https://reviewboard.mozilla.org/r/77500/#review75910
Attachment #8789208 - Flags: review?(jhofmann) → review+
Comment on attachment 8789203 [details]
Bug 1301287 - Password manager: enable eslint brace-style rule on shipping code.

https://reviewboard.mozilla.org/r/77490/#review75900

Yeah, that's the only reason and I care more about finding bugs and consistency in shipping code. I'm mostly looking for low-hanging fruit at the moment.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/66e80898e791
Password manager: enable eslint brace-style rule on shipping code. r=johannh
https://hg.mozilla.org/integration/autoland/rev/c2f550b20509
Remove useless method comments and convert others to JSDoc style. r=johannh
https://hg.mozilla.org/integration/autoland/rev/96afe7c27293
Password manager: enable eslint semi rule. r=johannh
https://hg.mozilla.org/integration/autoland/rev/1daf7a032230
Password manager: enable eslint space-infix-ops rule. r=johannh
https://hg.mozilla.org/integration/autoland/rev/4cfa5df42a78
Password manager: enable eslint no-new-object rule. r=johannh
https://hg.mozilla.org/integration/autoland/rev/4f7ca1523e06
Password manager: enable eslint no-array-constructor rule. r=johannh
You need to log in before you can comment on or make changes to this bug.