1517664 - Tidy some linting code in mail/components

Status: RESOLVED FIXED

(MailNews Core :: General, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Some of the linting I did early on in mail/components could be improved. Plus, somebody's come along and overridden some of the rules.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1517664-eslint-again-1.diff

Comment 2

[:aceman]

Comment on attachment 9034326 [details] [diff] [review]
1517664-eslint-again-1.diff

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

::: mail/components/about-support/content/aboutSupport.js
@@ +1084,2 @@
>          this._serializeElement(child);
> +      }

Sadly this code is more or less copied and kept in sync with m-c version of the file. Can you see what they do so that we do not differ (to ease porting) ?

::: mail/components/accountcreation/content/createInBackend.js
@@ -4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -
> -ChromeUtils.import("resource:///modules/MailServices.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");

We do import jsms in every file if the symbols are used. It maybe some .xul file loads all the js files together so importing the modules would be sufficient to do once, but who tracks those dependencies?
For jsms we tend to not care and just import everywhere, it is supposedly cheap.
For other global variables of course there must be no dupes.

::: mail/components/addrbook/content/addressbook.js
@@ -364,5 @@
>      }
>    }
>  
> -  /* eslint-disable no-global-assign */
> -  printEngineWindow = window.openDialog("chrome://messenger/content/msgPrintEngine.xul",

Why is it OK to remove assigning printEngineWindow ? The var seems to be defined in mailnews/base/content/msgPrintEngine.js but unused after assignment. Could you then remove it from all places?

Comment 3

[:aceman]

Comment on attachment 9034326 [details] [diff] [review]
1517664-eslint-again-1.diff

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

::: mail/components/accountcreation/content/createInBackend.js
@@ -4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -
> -ChromeUtils.import("resource:///modules/MailServices.jsm");
> -ChromeUtils.import("resource://gre/modules/Services.jsm");

I mean if you linked any of these files in some other xul file, the imports may then be needed.

Of course in the case here, all these js files are a group and will probably not be used anywhere else outside the account creation wizard. I think Ben can comment on this part.

Comment 4

[Ben Bucksch (:BenB)]

> all these js files are a group and will probably not be used anywhere else outside the account creation wizard.

I was hoping they would, actually :).

But we can still adapt eslint then at that time. No need to worry about that now.

- "Abortable": true,
...
+ import-globals-from

Nice!

Is it possible to put the "import-globals-from" all in one line, instead of making 12 lines for that?

+var {
+  cleanUpHostName,
+  isLegalHo

This is massively ugly. Could you please leave this as it was before?

Comment 5

[Ben Bucksch (:BenB)]

+        } else if (!config.incoming.hostname) {
+          config.incoming = server;
         } else {
-          if (!config.incoming.hostname) {
-            config.incoming = server;
-          } else {
-            config.incomingAlternatives.push(server);
-          }
+          config.incomingAlternatives.push(server);

Please leave this the way it was. I had intentionally written it like this. I know that eslint doesn't like it, but the way I wrote it makes the logic much easier to see and to read. (There are 2 cases: incoming and outgoing. Both are handled in the same way. The way I structured the code, it's obvious that incoming and outgoing are the same. The way eslint wants it is *much* much harder to read. That's why I deliberately and explicitly disabled the "no-lonely-if" eslint rule for this block. eslint is supposed to increase readability, and this specific rule does the opposite in this case here.

Comment 6

[Ben Bucksch (:BenB)]

before:
         if (type == "SMTP") {
           if (!config.outgoing.hostname) {
             config.outgoing = server;
           } else {
             config.outgoingAlternatives.push(server);
           }
         } else {
           if (!config.incoming.hostname) {
             config.incoming = server;
           } else {
             config.incomingAlternatives.push(server);
           }
         }
You can clearly see that we do the exact same thing for incoming and outgoing.

Things like that are also important to prevent bugs. If we find a bug in incoming or outgoing, and somebody changes the code for one of them, and doesn't realize from the code that the other case is identical, it may adapt only one and not the other, leading to unnecessary bugs. That's not a theoretical problem, I have seen that happen in reality many many times. Where code cannot be factored out, it's critically important to at least keep it identical, otherwise it becomes unmaintainable.

Comment 7

[Ben Bucksch (:BenB)]

createInBackend.js
- /* eslint-disable complexity */
/**
...
 * @return {nsIMsgAccount} - the newly created account
 */
+ /* eslint-disable complexity */
 function createAccountInBackend(config) {

This is a JSDoc function documentation. If you put eslint directives between the function signature and the comment right above it that documents the function, it breaks the JSDoc parser and any other JavaDoc-based documentation parser. With this new change, JSDoc (and the many similar parsers that also exist) can no longer recognize that the documentation belongs to the function.

This was wrong a few months ago, and I had fixed it, and this change breaks it again. Please make sure, throughout the codebase, to never put eslint comments between the function signature and the comment that documents the function.

Same in readFromXML.js. I had deliberately moved that, too.

Please revert those, it breaks various doc parsers.

sanitizeDataTypes.js
- ChromeUtils.import("resource:///modules/hostnameUtils.jsm");

Yes, this particular class was supposed to be reused elsewhere in the code base. It would be nice to keep this one self-contained.

Likewise, I think it would be better to keep the imports in util.js where they are used instead of moving them to emailWizard.js. Unless that makes things much harder for you... if it does, then OK. But if it doesn't make a big difference, I think it would be better to keep the imports in the file they are used. After all, that's how imports are supposed to work (in almost all languages I know).

However, sanitizeDatatypes.js is being used in isolation (I use it in almost all my projects), so that one is the most important.

Comment 8

[Ben Bucksch (:BenB)]

The other changes (brackets moved, subtitle format changed etc.) are OK with me.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1517664-eslint-again-2.diff

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Ben Bucksch (:BenB) from comment #4)

Is it possible to put the "import-globals-from" all in one line, instead of
making 12 lines for that?

No, it doesn't support that, which is sad.

+var {

• cleanUpHostName,
• isLegalHo

This is massively ugly. Could you please leave this as it was before?

I don't disagree it's ugly. It's how we're doing things for now. That JSM is one of the most annoying examples and is high on my list of things to fix.

(In reply to Ben Bucksch (:BenB) from comment #7)

This is a JSDoc function documentation. If you put eslint directives between
the function signature and the comment right above it that documents the
function, it breaks the JSDoc parser and any other JavaDoc-based
documentation parser.

I forgot about that, so I've gone back and fixed the other places I made the same mistake.

Comment 11

[Ben Bucksch (:BenB)]

Hey Goeff,

thanks for the responses.

Could you please also revert |if| change mentioned in comment 5/6 and the sanitizeDataTypes.js imports?

I don't disagree it's ugly.

Can you please just leave it as-is? It's not a problem to import all symbols from the module.

If you absolutely want to import only selected symbols, please put it in all in one line. If eslint doesn't allow it, please adapt the role globally, because that should be possible.

[JSDoc]
I've gone back and fixed the other places I made the same mistake.

Thank you! :)

Comment 12

[Ben Bucksch (:BenB)]

Could you please also revert |if| change mentioned in comment 5/6

Ah, I see you did that. Thank you!

Leaves the 2 comments:

• Keep sanitizeDatatypes.js imports local / autonomous (comment 7 part 2)
• Keep all imports on a single line (comment 11)

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1517664-eslint-again-3.diff

Comment 14

[Ben Bucksch (:BenB)]

Comment on attachment 9036232 [details] [diff] [review]
1517664-eslint-again-3.diff

Thanks, Geoff.

Comment 15

[:aceman]

Comment on attachment 9036232 [details] [diff] [review]
1517664-eslint-again-3.diff

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

Thanks, but it breaks the accountcreation xpcshell and mozmill tests (e.g. mozmill/newmailaccount/test-newmailaccount.js).

::: mail/components/accountcreation/content/readFromXML.js
@@ +213,5 @@
>  
>    d.inputFields = [];
>    for (let inputField of array_or_undef(xml.$inputField)) {
>      try {
> +      var fieldset = {

'let' could be better.

::: mail/components/accountcreation/content/sanitizeDatatypes.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* import-globals-from emailWizard.js */
> +/* globals cleanUpHostName, isLegalHostNameOrIP */
> +ChromeUtils.import("resource:///modules/hostnameUtils.jsm", null);

For some reason the test comm/mail/components/test/unit/test_autoconfigXML.js fails with this patch due to:
ERROR ReferenceError: cleanUpHostName is not defined at chrome://messenger/content/accountcreation/sanitizeDatatypes.js:90

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1517664-eslint-again-4.diff

Up to you if you read this again or just look at the interdiff, but I know which I'd do. :)

Comment 17

[:aceman]

Comment on attachment 9038731 [details] [diff] [review]
1517664-eslint-again-4.diff

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

Thanks, this works.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/881cf0d594f2
Tidy some linting code in mail/components; r=aceman

Comment 19

[Geoff Lankow (:darktrojan)]

It seems I upset the delicate balance of circular import-globals-from references. Added a few more to fix.