Closed Bug 1488607 Opened Last year Closed Last year

Adapt to ESLint rule changes in bug 1488445

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(1 file)

M-C have updated the recommended rules. The only one affecting us is no-native-reassign replaced with no-global-assign.
I've also cleaned up a redundant rule in mailnews.
Attachment #9006414 - Flags: review?(jorgk)
Comment on attachment 9006414 [details] [diff] [review]
1488607-eslint-1.diff

Thanks. I had noticed the redundant rule in mailnews/, but I was tired after a 300+KB patch review. Thanks for cleaning up.
Attachment #9006414 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4a2af5ca975b
Adapt to ESLint rule changes in bug 1488445. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
(In reply to Geoff Lankow (:darktrojan) from comment #1)
> Created attachment 9006414 [details] [diff] [review]
> 1488607-eslint-1.diff
> 
> I've also cleaned up a redundant rule in mailnews.

Did you verify this? The following no longer raises an error locally if a dangling comma is omitted:
/mozilla-central/comm$ ../mach lint mailnews/extensions/newsblog/

There is an override in mozilla-central for files /extensions/** and comma-dangle off.
> $ ../mach lint mailnews/extensions/newsblog/
> /home/geoff/mozilla/mozilla-central/comm/mailnews/extensions/newsblog/js/newsblog.js
>  32:77  error  Missing trailing comma.  comma-dangle (eslint)
>
> ✖ 1 problem (1 error, 0 warnings)

Is your mozilla-central up-to-date?
m-c tip is c4c125ee2556, c-c tip is 5b9052faa6b4.  Removing the comma in the same place as above raises no error.
Please revert the change and restore the comma-dangle rule in my original patch. If you reread comment 4 you will see why, here let me show you - https://searchfox.org/comm-central/rev/27b3d9c5fd2543f47c3943590c72fa560a5b6b9a/mozilla/.eslintrc.js#43. You may also want to check your m-c up-to-dateness.

I would suggest better etiquette is more NIing and less gunslinging..
Flags: needinfo?(geoff)
Alta88, the link you gave doesn't work. It's preferable to point to M-C files via "mozilla-central" and skip the "mozilla" bit.

I don't really want to get involved in what looks like a bit of friction ;-) - but to follow up, Geoff did this try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=173433f969c978b91e11fdaf060005e7f9398bf7
Removing the comma here
https://hg.mozilla.org/try-comm-central/rev/173433f969c978b91e11fdaf060005e7f9398bf7#l3.12
does trigger the rule.

I cannot confirm your comment #4 locally: ../mach lint mailnews/extensions/newsblog/ gives me
c:\mozilla-source\comm-central\comm\mailnews\extensions\newsblog\js\newsblog.js
  32:77  error  Missing trailing comma.  comma-dangle (eslint)

So may I consider this issue as resolved or am I missing something?
Flags: needinfo?(geoff)
https://dxr.mozilla.org/mozilla-central/source/.eslintrc.js#43 overrides /extensions absent a specific rule lower in the comm subdir. So it does not work for me, with latest tips on m-c and c-c, no. Do you not locally have that m-c/eslintrc.js as in dxr?
I'm on tip most of the time since I'm doing bustage fixes. Yes, the DXR link reflects what I have locally. It's working for me locally, I've just verified that again. Does M-C's .eslintrc.js get applied? C-C has it's own. Sorry I can't be of further help.
Changing that rule in m-c/.eslintrc.js locally has no effect, I have no idea what's going on or why if wfy. Maybe you have a global eslint setup, in /home or elsewhere. I used to have one and removed it. Maybe I need fresh clones. In any case, thanks for trying to help, rather than trampling my patch, making it not wfm, and walking away.
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.