Closed Bug 1549166 Opened 6 months ago Closed 5 months ago

Turn on ESLint in all of comm-central

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(3 files)

I'm going to use this bug for a few clean-up tasks remaining. All that's left to be linted is mailnews (bug 1515877), ldap, and mail/test/mozmill.

After that I'm going to clean up the .eslintignore file and adjust the top config file to eliminate all the identical config files I've left in test directories everywhere.

Type: defect → task

This is the most difficult directory.

Attachment #9062775 - Flags: review?(jorgk)
Comment on attachment 9062775 [details] [diff] [review]
1549166-eslint-ldap-1.diff

Review of attachment 9062775 [details] [diff] [review]:
-----------------------------------------------------------------

I am in fact *the best* reviewer for this since I used the test as basis for https://searchfox.org/comm-central/source/mailnews/local/test/unit/test_mailboxURL.js.

::: ldap/xpcom/tests/unit/test_nsLDAPURL.js
@@ +2,5 @@
>  /*
>   * Test suite for nsLDAPURL functions.
>   */
>  
> +var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

You need this? The mailbox URL test doesn't have that import.
Attachment #9062775 - Flags: review?(jorgk) → review+

You need this? The mailbox URL test doesn't have that import.

It has a head file which imports Services.

Keywords: checkin-needed
Keywords: checkin-needed
Depends on: 1550674

This is a copy of the patch in bug 1415265.

Attachment #9064952 - Flags: review?(acelists)
Comment on attachment 9064952 [details] [diff] [review]
1549166-eslint-config-1.diff

Review of attachment 9064952 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: .eslintrc.js
@@ +80,5 @@
> +  }, {
> +    // If it is an xpcshell head file, we turn off global unused variable checks, as it
> +    // would require searching the other test files to know if they are used or not.
> +    // This would be expensive and slow, and it isn't worth it for head files.
> +    // We could get developers to declare as exported, but that doesn't seem worth it.

Is this missing some word?
Attachment #9064952 - Flags: review?(acelists) → review+
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/13665dfc61d8
Centralise configuration of ESLint environments for test directories; r=aceman
Keywords: leave-open
Target Milestone: --- → Thunderbird 68.0

Chat tests use slightly different paths, so this adds another path entry to the root .estlintrc.js and deletes a bunch more .eslintrc.js files from chat.

Attachment #9066099 - Flags: review?(geoff)
Comment on attachment 9066099 [details] [diff] [review]
Use centralized configs for chat

Nice one, thanks Patrick. I wanted to remove these files but didn't have a reasonable way to do it. This is as good as anything I came up with.
Attachment #9066099 - Flags: review?(geoff) → review+
Keywords: checkin-needed

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/9efee8d57a4c
Use centralized ESLint configuration for chat tests. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

https://hg.mozilla.org/releases/comm-beta/rev/f86e8e7e79023267bbf1b1e7e6db9c21ca3f5a4d

The last patch joined its friends on beta, also to trigger a build which didn't happen after the merge.

Just to make it clear, the bug summary mentions all of comm-central, but in none of the bugs I have seen changes to Seamonkey (/suite subfolder). So it should probably be stated that that was an explicit non-goal of this cleanup and /suite is NOT being eslinted.

(In reply to Geoff Lankow (:darktrojan) from comment #9)

Nice one, thanks Patrick. I wanted to remove these files but didn't have a
reasonable way to do it. This is as good as anything I came up with.

It isn't perfect, but it was the best I could come up with! Thanks for the review (and thanks for checking in / uplifting Jorg!)

You need to log in before you can comment on or make changes to this bug.