Closed Bug 1426240 Opened 3 years ago Closed 3 years ago

Linting problem: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/extensionSupport.jsm:68:24 | 'fixIterator' is not defined.

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

Strange, since it is defined in iteratorUtils.jsm which is imported. What am I missing?
Flags: needinfo?(philipp)
Flags: needinfo?(acelists)
Summary: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/extensionSupport.jsm:68:24 | 'fixIterator' is not defined. → Linting problem: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/common/src/extensionSupport.jsm:68:24 | 'fixIterator' is not defined.
Can the linter see inside .jsms what objects are imported? But if this would be a problem, almost all .js files importing jsms would be flagged in this way. So how is extensionSupport.jsm special?
walkExtensionPrefs() is called async from the addonmanager. May it be that the imported objects are out of scope at that time? But we know locally that the code works and does not complain about the missing fixIterator.
Flags: needinfo?(acelists)
Actually case 1 could be it, we just do not see the problem in other files because all others are excluded from linting. But common/* is a new folder and is missing from .eslintignore yet. Of course I'd like we actually enable linting of all our files one day.
So we may keep this bug to determine what needs to be done to solve this particular type of problem.
But then also IOUtils should have been reported...
I have got the ES linter green with https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f4095b3b5fb0ea9728c8232f9ac38ee86296bf8f but I still do not understand why it does not complain about IOUtils too. Is it a difference that IOUtils.js exports a single object (named the same), but iteratorUtils.jsm does three?
I think you should add the new directory to eslintignore. If you want to enable a new directory, you will have to define a number of linting rules and agree on style, as I did in calendar/.eslintrc.js. Lightning disables this specific rule because I thought the globals importing stuff was messy.
Flags: needinfo?(philipp)
Blocks: 1426459
Thanks, yes I think the linter does not see imported objects so we could turn off this rule globally for now. See bug 1426459 for the mess it outputs.

I would leave this directory enabled to be linted as it contains a single file and we could try to keep it without warnings as a testbed. We could try to go with the defaults and see what happens, e.g. there are no other warnings in this file, so our current style seems fine for the linter.
Having everything that is being imported named specifically certainly makes it easier to see where stuff comes from. I know most python projects I've worked on tend to frown on 'from module import *' for exactly that reason.
Attached patch 1426240.patch (obsolete) β€” β€” Splinter Review
Turning off no-undef seems to help much in general.
Attachment #8938268 - Flags: review?(philipp)
Comment on attachment 8938268 [details] [diff] [review]
1426240.patch

I would not want to turn this on globally, such decisions should be per-subdirectory. You are welcome to add this to common/.eslintrc.js though.
Attachment #8938268 - Flags: review?(philipp) → review-
Attached patch 1426240.patch v2 (obsolete) β€” β€” Splinter Review
Ok, moved the setting under /common .
Assignee: nobody → acelists
Attachment #8938268 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8938674 - Flags: review?(philipp)
Comment on attachment 8938674 [details] [diff] [review]
1426240.patch v2

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

r+wc

::: common/.eslintrc.js
@@ +4,5 @@
> +  // We would like the same base rules as provided by
> +  // mozilla/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
> +  "extends": [
> +    "plugin:mozilla/recommended"
> +  ],

This is already inherited by the toplevel .eslintrc.js, all you need here is the rules section.
Attachment #8938674 - Flags: review?(philipp) → review+
Attached patch 1426240.patch v2.1 β€” β€” Splinter Review
Thanks.
Attachment #8938674 - Attachment is obsolete: true
Attachment #8938686 - Flags: review+
Keywords: checkin-needed
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Target Milestone: --- → Thunderbird 59.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/63658572abf9
turn off linter warning of undefined identifiers due to too many false positives in /common. r=philipp
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.