Closed Bug 1346671 Opened 7 years ago Closed 7 years ago

Enable the no-useless-concat eslint rule in toolkit/

Categories

(Toolkit :: General, defect)

Product:
Toolkit
Component:
General
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: squib, Assigned: squib)

References

Details

Attachments

(1 file)

Bug 1346671 - Enable the no-useless-concat eslint rule in toolkit/
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Should be pretty self-explanatory. Patch forthcoming.
Comment on attachment 8846905 [details]
Bug 1346671 - Enable the no-useless-concat eslint rule in toolkit/

This should be a relatively straightforward review. I haven't pushed to try yet though, so it's possible I broke something. I'm still figuring out all the various stuff the firefoxtree HG extension can do...

I'll hopefully update this bug soon with try results.
Attachment #8846905 - Flags: review?(jaws)
Ok, I think I have a try run kicked off via ReviewBoard.
Comment on attachment 8846905 [details]
Bug 1346671 - Enable the no-useless-concat eslint rule in toolkit/

https://reviewboard.mozilla.org/r/119896/#review121804

::: toolkit/.eslintrc.js:244
(Diff revision 1)
> +
> +    "no-useless-concat": "error",

We typically add a one-line explanation of what the rule does here.
Attachment #8846905 - Flags: review?(jaws) → review+
I checked the try run and got some eslint errors in /browser since it inherits from /toolkit's .eslintrc.js. Should I update /browser too or should I just override the no-useless-concat setting in /browser? If the former, do I need another set of eyes on this?
Flags: needinfo?(jaws)
How many errors are there in /browser? If possible we should just fix the ones in /browser as part of this. We have traditionally done that elsewhere if the number of errors is manageable.
Flags: needinfo?(jaws) → needinfo?(squibblyflabbetydoo)
Just 5 of them: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b580b332437&selectedJob=83547272>. I have a fix in my local repo that I can push up for review if that's how we want to go.
Flags: needinfo?(squibblyflabbetydoo)
Yeah, that's perfect. Thanks!
Comment on attachment 8846905 [details]
Bug 1346671 - Enable the no-useless-concat eslint rule in toolkit/

Ok, this should be done, and I think it'll pass try this time around.
Attachment #8846905 - Flags: review?(jaws)
Whoops, missed one more in /security/manager/ssl/tests! The current patch on MozReview has that fixed too.
Comment on attachment 8846905 [details]
Bug 1346671 - Enable the no-useless-concat eslint rule in toolkit/

https://reviewboard.mozilla.org/r/119896/#review122326
Attachment #8846905 - Flags: review?(jaws) → review+
Blocks: eslint
No longer depends on: eslint
Pushed by squibblyflabbetydoo@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b8f82003bf72
Enable the no-useless-concat eslint rule in toolkit/ r=jaws
Landed. In Thunderbird at least, the rule was that we set the Milestone field when landing a bug, but I don't see that in the new Bugzilla. Are there any tracking flags I should be setting when I resolve a bug?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jaws)
Flags: in-testsuite+
Resolution: --- → FIXED
Nope, no need to resolve the bug manually. It will get resolved once the patch gets merged to mozilla-central.
Status: RESOLVED → REOPENED
Flags: needinfo?(jaws)
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/b8f82003bf72
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: