1275096 - Use regex find/replace to fix Hungarian notation ESLint warnings

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Tim Nguyen :ntim]

Doing a quick find/replace seems effective in most long files I've tried.

The following works pretty well (in Sublime Text) when I tried it in view-helpers.js (which was filled with `aArgs`):

Find: a([A-Z]\w+)
Replace with: \L$1

I think we should do this across the devtools/ directory.

Comment 1

[Tim Nguyen :ntim]

Thoughts ?

Comment 2

[Tim Nguyen :ntim]

So it seems to have some false positives (camelcased function names, within strings), but I think the regex can be adjusted to take in account those as well.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

What happens to multi-word things like aMultiWord?  It should become multiWord, not multiword.  You may need \l instead of \L if I am interpreting docs correctly.

Anyway, the concept sounds good to me.

Comment 4

[Patrick Brosset <:pbro>]

If we can fix these automatically, that'd be great.
As you may know, we have a custom eslint rule that warns against using them:
https://dxr.mozilla.org/mozilla-central/source/testing/eslint/eslint-plugin-mozilla/lib/rules/no-aArgs.js
This rule even makes a suggestion.
Furthermore, for some time, ESLint has supported fixable rules. So we could experiment with making this rule support auto-fixing.
It doesn't seem hard to do looking at one of ESLint's fixable rules: https://github.com/eslint/eslint/blob/42d1ecc9077328153f46dc5b6f972806777ec168/lib/rules/quotes.js#L231

Now, of course, there's going to be problems when both aArg and arg variables are present in the same scope, which does happen in some cases, like (and I've seen this many times in the code):

let arg = null;
function doSomething(aArg) {
  arg = aArg;
}

You just can't autofix aArg to arg here, and that's when the fix is hard because it requires to manually do it and it requires to actually know what aArg really is so that you can give it a good name (because, yeah, we could make the auto-fixer list variables in the scope and if it found arg, it could then choose arg2, arg3, etc..., but that would be really awful).
Maybe the auto-fixer could fix the majority of cases when there are no local variables with the suggested name only, and leave all the other cases alone. That would be a start.

Comment 5

[Tom Tromey :tromey]

(In reply to Tim Nguyen :ntim (not accepting requests) from comment #1)
> Thoughts ?

It's a good idea.  Patrick notes the major possible issue.
Also I would recommend splitting this and going directory-by-directory or so.
Otherwise the resulting patch will be a nightmare for the reviewer.

Comment 6

[Mark Banner (:standard8)]

Note: mozilla/no-aArgs is mostly enabled for devtools, just needs enabling for the remaining directories listed here:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/devtools/.eslintrc.js#71-80

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1275096 - [devtools] Fix eslint no aArgs violations

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b5a1d0ac8578
https://hg.mozilla.org/integration/autoland/rev/ffe22635b156
[devtools] Fix eslint no aArgs violations r=frontend-codestyle-reviewers,devtools-reviewers,nchevobbe

Comment 9

[pstanciu]

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