Open
Bug 1275096
Opened 8 years ago
Updated 2 years ago
Use regex find/replace to fix Hungarian notation ESLint warnings
Categories
(DevTools :: General, enhancement)
DevTools
General
Tracking
(Not tracked)
NEW
People
(Reporter: ntim, Unassigned)
References
(Blocks 1 open bug)
Details
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.
Reporter | ||
Comment 1•8 years ago
|
||
Thoughts ?
Flags: needinfo?(ttromey)
Flags: needinfo?(pbrosset)
Flags: needinfo?(jryans)
Reporter | ||
Comment 2•8 years ago
|
||
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.
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.
Flags: needinfo?(jryans)
Comment 4•8 years ago
|
||
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.
Flags: needinfo?(pbrosset)
Severity: normal → enhancement
Comment 5•8 years ago
|
||
(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.
Flags: needinfo?(ttromey)
Updated•7 years ago
|
Blocks: dt-polish-debt
Updated•7 years ago
|
No longer blocks: dt-polish-debt
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 6•6 years ago
|
||
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
Updated•6 years ago
|
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•