Closed Bug 1372427 Opened 4 years ago Closed 4 years ago

Enable more eslint rules for satchel

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jonathanGB, Assigned: jonathanGB)

Details

Attachments

(25 files, 1 obsolete file)

Bug 1372427 - add semi-colon rule.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - Add indentation rules.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add js-doc rule.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add "block-scoped-var" rule. .
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add "no-use-before-define" rule.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add cyclomatic-complexity limit.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add "dot-location" rule so that dots are on the same line as properties.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - limit lines to a maximum of 100 chars.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add rule "no-fallthrough" in switch cases. .
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - limit to 2 consecutive empty lines.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add rule to not throw litterals.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add error to not pad blocks.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - force radix argument in parseInt.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - enforce "var" declarations only at top-level.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add rule to not put spacing inside arrays.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add rule to not put spacing inside parens.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - enforce "comma-dangle".
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - enforce dot-notation over square-bracket-notation when possible.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - enforce no spacing before semi-colons, but spacing after.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add rest of rules which don't affect the code.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - add "no-unused-vars" rule, with laxist variation in head_satchel + satchel_common.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - prevent using arguments.caller.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - enforce balanced-listeners.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - Add "curly" eslint rule to fix missing curly braces. .
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1372427 - re-order rules in alphabetical order.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Currently, the root of satchel has no `.eslintrc.js` defined. We should add rules and other restrictions to normalize satchel (indentation, curly braces required, etc.)
Comment on attachment 8876938 [details]
Bug 1372427 - Add indentation rules.

https://reviewboard.mozilla.org/r/148266/#review154302

It would be great to know which commits onlly contain new rules plus --fix applied. Perhaps you could add that to line 2+ of the commit message?

::: toolkit/components/satchel/.eslintrc.js:6
(Diff revision 2)
> +    indent: ["error", 2, {
> +      SwitchCase: 1,

So the problem is that when this runs in automation or on mach it's going to fail since the test files aren't following this rule. Can you edit this commit to disable it for the test directories or else have it also fix the test directories? (I think the latter may bitrot other patches more though).

::: toolkit/components/satchel/nsFormAutoCompleteResult.jsm:27
(Diff revision 2)
> -
> -  if (prevResult) {
> -    this.entries = prevResult.wrappedJSObject.entries;
> +  this.entries = prevResult ?
> +    prevResult.wrappedJSObject.entries :
> +    [];

Why not all on one line?

::: toolkit/components/satchel/test/unit/test_autocomplete.js:44
(Diff revision 2)
> -    fac = Cc["@mozilla.org/satchel/form-autocomplete;1"].
> +  fac = Cc["@mozilla.org/satchel/form-autocomplete;1"].
>            getService(Ci.nsIFormAutoComplete);
> -    prefs = Cc["@mozilla.org/preferences-service;1"].
> +  prefs = Cc["@mozilla.org/preferences-service;1"].
>              getService(Ci.nsIPrefBranch);

Hmm… I kinda prefer aligning the beginnings but really I think these lines would be <100 characters so these could go on one line.
Attachment #8876938 - Flags: review?(MattN+bmo)
Comment on attachment 8876937 [details]
Bug 1372427 - add semi-colon rule.

https://reviewboard.mozilla.org/r/148264/#review154306
Attachment #8876937 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877349 [details]
Bug 1372427 - add js-doc rule.

https://reviewboard.mozilla.org/r/148732/#review154308

::: toolkit/components/satchel/FormHistory.jsm:556
(Diff revision 2)
>  /**
>   * dbAreExpectedColumnsPresent
>   *
>   * Sanity check to ensure that the columns this version of the code expects

For another bug can you remove all cases of putting the function name at the top of the JSDoc comment… I find it very redundant.

::: toolkit/components/satchel/nsFormAutoComplete.js:46
(Diff revision 2)
> - * @param Object with the following properties:
> - *
> - *        formField (DOM node):
> - *          A DOM node that we're requesting form history for.
> + * @param {Node} formField A DOM node that we're requesting form history for.
> + * @param {string} inputName The name of the input to do the FormHistory look-up with.
> + *                           If this is searchbar-history, then formField needs to be null,
> + *                           otherwise constructing will throw.

Does this pass? IIUC, there is a better way to document that it's an object with two properties that get destructured.
Comment on attachment 8877826 [details]
Bug 1372427 - add error to not pad blocks.

https://reviewboard.mozilla.org/r/149240/#review154314

::: toolkit/components/satchel/.eslintrc.js:49
(Diff revision 1)
>      "no-fallthrough": "error",
>      "no-multiple-empty-lines": ["warn", {
>        max: 2
>      }],
>      "no-throw-literal": "error",
> +    "padded-blocks": ["warn", "never"],

We're trying to not use errors as they don't cause a failure upon checking. For new rules please use error
Attachment #8877826 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877832 [details]
Bug 1372427 - enforce dot-notation over square-bracket-notation when possible.

https://reviewboard.mozilla.org/r/149252/#review154316

::: toolkit/components/satchel/FormHistory.jsm:487
(Diff revision 1)
>      if (!_dbConnection.tableExists("moz_deleted_formhistory")) {
> -      let table = dbSchema.tables["moz_deleted_formhistory"];
> +      let table = dbSchema.tables.moz_deleted_formhistory;
>        let tSQL = Object.keys(table).map(col => [col, table[col]].join(" ")).join(", ");

I honestly think the old way was more readable but that's because dbSchema.tables should have been a `Map`
Attachment #8877832 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877827 [details]
Bug 1372427 - force radix argument in parseInt.

https://reviewboard.mozilla.org/r/149242/#review154318
Attachment #8877827 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877825 [details]
Bug 1372427 - add rule to not throw litterals.

https://reviewboard.mozilla.org/r/149238/#review154322
Attachment #8877825 - Flags: review?(MattN+bmo) → review+
Priority: -- → P3
Summary: Add eslint to satchel → Enable more eslint rules for satchel
Attachment #8877213 - Attachment is obsolete: true
Attachment #8877213 - Flags: review?(MattN+bmo)
Comment on attachment 8876938 [details]
Bug 1372427 - Add indentation rules.

https://reviewboard.mozilla.org/r/148266/#review155772
Attachment #8876938 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877349 [details]
Bug 1372427 - add js-doc rule.

https://reviewboard.mozilla.org/r/148732/#review155774

::: toolkit/components/satchel/.eslintrc.js:16
(Diff revision 5)
>        },
> -      MemberExpression: "off",
> +      //MemberExpression: "off",
>        outerIIFEBody: 0,

This doesn't belong in this commmit
Attachment #8877349 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877350 [details]
Bug 1372427 - add "block-scoped-var" rule. .

https://reviewboard.mozilla.org/r/148734/#review155778

::: toolkit/components/satchel/.eslintrc.js:19
(Diff revision 5)
>      }],
> -    "semi": ["error", "always"],
> +    semi: ["error", "always"],
>      "valid-jsdoc": ["error", {

This is also in the wrong commit
Attachment #8877350 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877351 [details]
Bug 1372427 - add "no-use-before-define" rule.

https://reviewboard.mozilla.org/r/148736/#review155782
Attachment #8877351 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877352 [details]
Bug 1372427 - add cyclomatic-complexity limit.

https://reviewboard.mozilla.org/r/148738/#review155786

::: toolkit/components/satchel/nsFormAutoComplete.js
(Diff revision 5)
> -    if (!this._enabled) {
> -      if (aListener) {
> -        aListener.onSearchCompletion(emptyResult);
> -      }
> -      return;
> -    }

Personally I like the older version of this file better so would probably not make this change but if you prefer it then it's fine.

::: toolkit/components/satchel/nsFormAutoComplete.js:303
(Diff revision 5)
> -        aListener.onSearchCompletion(emptyResult);
> -      }
> -      return;
> -    }
>  
> -    // don't allow form inputs (aField != null) to get results from search bar history
> +    let earlyReturnflag = false;

Nit: Maybe just `earlyReturn` or `earlyReturnFlag` (capital "F")
Attachment #8877352 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877353 [details]
Bug 1372427 - add "dot-location" rule so that dots are on the same line as properties.

https://reviewboard.mozilla.org/r/148740/#review155790

::: toolkit/components/satchel/test/unit/test_notify.js:66
(Diff revision 5)
> -    var os = Cc["@mozilla.org/observer-service;1"].
> -         getService(Ci.nsIObserverService);
> +    var os = Cc["@mozilla.org/observer-service;1"]
> +             .getService(Ci.nsIObserverService);

Nit: put this on one line instead
Attachment #8877353 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877354 [details]
Bug 1372427 - limit lines to a maximum of 100 chars.

https://reviewboard.mozilla.org/r/148742/#review155798

::: toolkit/components/satchel/.eslintrc.js:35
(Diff revision 5)
>      "block-scoped-var": "error",
>      "no-use-before-define": ["error", {
>        functions: false,
>      }],
>      complexity: ["error", {
>        max: 20,
>      }],
>      "dot-location": ["error", "property"],
> +    "max-len": ["error", 100],

In the future please keep any new lists in alphabetical order. You can fix this in a new commit at the end so you don't have to redo all of the intermediate commits

::: toolkit/components/satchel/test/test_bug_511615.html:121
(Diff revision 5)
>    /* eslint-disable no-multi-spaces */
> -  dnEvent.initMouseEvent("mousedown",  true, true, window, 1, 0, 0, 0, 0, false, false, false, false, 0, null);
> -  upEvent.initMouseEvent("mouseup",    true, true, window, 1, 0, 0, 0, 0, false, false, false, false, 0, null);
> -  ckEvent.initMouseEvent("mouseclick", true, true, window, 1, 0, 0, 0, 0, false, false, false, false, 0, null);
> +  dnEvent.initMouseEvent("mousedown",  true, true, window, 1, 0, 0, 0, 0,
> +                         false, false, false, false, 0, null);
> +  upEvent.initMouseEvent("mouseup",    true, true, window, 1, 0, 0, 0, 0,
> +                         false, false, false, false, 0, null);
> +  ckEvent.initMouseEvent("mouseclick", true, true, window, 1, 0, 0, 0, 0,

I think it's better to also disable the rule here… the point of aligning the arguments was to make it easier to compare the lines but the wrapping kinda defeats that.

::: toolkit/components/satchel/test/test_form_submission.html:408
(Diff revision 5)
> -      checkForSave("test4", "trimTrailingAndLeadingSpace", "checking saved value is trimmed on both sides");
> +      checkForSave("test4", "trimTrailingAndLeadingSpace", "checking saved value is trimmed on " +
> +                   "both sides");
>        break;
>      case 105:
> -      checkForSave("test5", "trimTrailingAndLeadingWhitespace", "checking saved value is trimmed on both sides");
> +      checkForSave("test5", "trimTrailingAndLeadingWhitespace", "checking saved value is " +
> +                   "trimmed on both sides");

Wrap between the arguments instead of in the middle of the strings

::: toolkit/components/satchel/test/test_form_submission_cap2.html:169
(Diff revision 5)
> -  checkForSave("test" + numInputFields, numInputFields + " changed", "checking saved value " + numInputFields);
> +  checkForSave("test" + numInputFields, numInputFields + " changed", "checking saved value " +
> +               numInputFields);

Same here, wrap after the comma (between arguments) instead
Attachment #8877354 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8879350 [details]
Bug 1372427 - Add "curly" eslint rule to fix missing curly braces. .

https://reviewboard.mozilla.org/r/150636/#review155810
Attachment #8879350 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877355 [details]
Bug 1372427 - add rule "no-fallthrough" in switch cases. .

https://reviewboard.mozilla.org/r/148744/#review155812
Attachment #8877355 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877824 [details]
Bug 1372427 - limit to 2 consecutive empty lines.

https://reviewboard.mozilla.org/r/149236/#review155814
Attachment #8877824 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877828 [details]
Bug 1372427 - enforce "var" declarations only at top-level.

https://reviewboard.mozilla.org/r/149244/#review155816
Attachment #8877828 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877829 [details]
Bug 1372427 - add rule to not put spacing inside arrays.

https://reviewboard.mozilla.org/r/149246/#review155818
Attachment #8877829 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877830 [details]
Bug 1372427 - add rule to not put spacing inside parens.

https://reviewboard.mozilla.org/r/149248/#review155820
Attachment #8877830 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877831 [details]
Bug 1372427 - enforce "comma-dangle".

https://reviewboard.mozilla.org/r/149250/#review155822
Attachment #8877831 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877833 [details]
Bug 1372427 - enforce no spacing before semi-colons, but spacing after.

https://reviewboard.mozilla.org/r/149254/#review155824
Attachment #8877833 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877834 [details]
Bug 1372427 - add rest of rules which don't affect the code.

https://reviewboard.mozilla.org/r/149256/#review155826
Attachment #8877834 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8878731 [details]
Bug 1372427 - add "no-unused-vars" rule, with laxist variation in head_satchel + satchel_common.

https://reviewboard.mozilla.org/r/150052/#review155828
Attachment #8878731 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8878732 [details]
Bug 1372427 - prevent using arguments.caller.

https://reviewboard.mozilla.org/r/150054/#review155830
Attachment #8878732 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8878733 [details]
Bug 1372427 - enforce balanced-listeners.

https://reviewboard.mozilla.org/r/150056/#review155832

::: toolkit/components/satchel/.eslintrc.js:20
(Diff revision 3)
>        },
> +      // XXX: following line is used in eslint v4 to not throw an error when chaining methods
>        //MemberExpression: "off",
>        outerIIFEBody: 0,

Please move this to the indent commit
Attachment #8878733 - Flags: review?(MattN+bmo) → review+
Please address the issues, request a try push for mochitests and xpcshell, then add the checkin-needed keyword if the try push is green. Thanks!
Flags: needinfo?(jguillotteblouin)
Comment on attachment 8879754 [details]
Bug 1372427 - re-order rules in alphabetical order.

https://reviewboard.mozilla.org/r/151104/#review155960

Thanks
Attachment #8879754 - Flags: review?(MattN+bmo) → review+
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: waiting for lock on working directory of /repo/hg/mozilla/try held by process '5270' on host 'hgssh4.dmz.scl3.mozilla.com'
remote: abort: working directory of /repo/hg/mozilla/try: timed out waiting for lock held by hgssh4.dmz.scl3.mozilla.com:5270
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Flags: needinfo?(jguillotteblouin)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06299c535903
Add "curly" eslint rule to fix missing curly braces. r=MattN.
https://hg.mozilla.org/integration/autoland/rev/7adcef1c36ab
Add indentation rules. r=MattN
https://hg.mozilla.org/integration/autoland/rev/85afcde5f5c8
add semi-colon rule. r=MattN
https://hg.mozilla.org/integration/autoland/rev/0df920a014c2
add js-doc rule. r=MattN
https://hg.mozilla.org/integration/autoland/rev/c8b79ee9095e
add "block-scoped-var" rule. r=MattN.
https://hg.mozilla.org/integration/autoland/rev/b94ed45ce47d
add "no-use-before-define" rule. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7ed835c64ffb
add cyclomatic-complexity limit. r=MattN
https://hg.mozilla.org/integration/autoland/rev/890dbe8e267d
add "dot-location" rule so that dots are on the same line as properties. r=MattN
https://hg.mozilla.org/integration/autoland/rev/3069e08d8403
limit lines to a maximum of 100 chars. r=MattN
https://hg.mozilla.org/integration/autoland/rev/b8e742aec436
add rule "no-fallthrough" in switch cases. r=MattN.
https://hg.mozilla.org/integration/autoland/rev/12563e754c2b
limit to 2 consecutive empty lines. r=MattN
https://hg.mozilla.org/integration/autoland/rev/584619a8d366
add rule to not throw litterals. r=MattN
https://hg.mozilla.org/integration/autoland/rev/abe6fe879f98
add error to not pad blocks. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5fb488d7a1e1
force radix argument in parseInt. r=MattN
https://hg.mozilla.org/integration/autoland/rev/a33fbea69391
enforce "var" declarations only at top-level. r=MattN
https://hg.mozilla.org/integration/autoland/rev/1de9c691fc69
add rule to not put spacing inside arrays. r=MattN
https://hg.mozilla.org/integration/autoland/rev/d31f39def749
add rule to not put spacing inside parens. r=MattN
https://hg.mozilla.org/integration/autoland/rev/df3bdaacb264
enforce "comma-dangle". r=MattN
https://hg.mozilla.org/integration/autoland/rev/f142f099e0aa
enforce dot-notation over square-bracket-notation when possible. r=MattN
https://hg.mozilla.org/integration/autoland/rev/ccbb9a0a1a3e
enforce no spacing before semi-colons, but spacing after. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8b577f09637f
add rest of rules which don't affect the code. r=MattN
https://hg.mozilla.org/integration/autoland/rev/40f7221dea20
add "no-unused-vars" rule, with laxist variation in head_satchel + satchel_common. r=MattN
https://hg.mozilla.org/integration/autoland/rev/b6ff7acf92d1
prevent using arguments.caller. r=MattN
https://hg.mozilla.org/integration/autoland/rev/50d6e8aac0e7
enforce balanced-listeners. r=MattN
https://hg.mozilla.org/integration/autoland/rev/3d24a37e1aa5
re-order rules in alphabetical order. r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06299c535903
https://hg.mozilla.org/mozilla-central/rev/7adcef1c36ab
https://hg.mozilla.org/mozilla-central/rev/85afcde5f5c8
https://hg.mozilla.org/mozilla-central/rev/0df920a014c2
https://hg.mozilla.org/mozilla-central/rev/c8b79ee9095e
https://hg.mozilla.org/mozilla-central/rev/b94ed45ce47d
https://hg.mozilla.org/mozilla-central/rev/7ed835c64ffb
https://hg.mozilla.org/mozilla-central/rev/890dbe8e267d
https://hg.mozilla.org/mozilla-central/rev/3069e08d8403
https://hg.mozilla.org/mozilla-central/rev/b8e742aec436
https://hg.mozilla.org/mozilla-central/rev/12563e754c2b
https://hg.mozilla.org/mozilla-central/rev/584619a8d366
https://hg.mozilla.org/mozilla-central/rev/abe6fe879f98
https://hg.mozilla.org/mozilla-central/rev/5fb488d7a1e1
https://hg.mozilla.org/mozilla-central/rev/a33fbea69391
https://hg.mozilla.org/mozilla-central/rev/1de9c691fc69
https://hg.mozilla.org/mozilla-central/rev/d31f39def749
https://hg.mozilla.org/mozilla-central/rev/df3bdaacb264
https://hg.mozilla.org/mozilla-central/rev/f142f099e0aa
https://hg.mozilla.org/mozilla-central/rev/ccbb9a0a1a3e
https://hg.mozilla.org/mozilla-central/rev/8b577f09637f
https://hg.mozilla.org/mozilla-central/rev/40f7221dea20
https://hg.mozilla.org/mozilla-central/rev/b6ff7acf92d1
https://hg.mozilla.org/mozilla-central/rev/50d6e8aac0e7
https://hg.mozilla.org/mozilla-central/rev/3d24a37e1aa5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.