1399756 - Code clean-up: Style nits, typos and trailing spaces - take 2

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1393219 +++

To facilitate continuous integration, I will collect some patches here that can be landed if no other patch is available to trigger a build.

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/24067b756994
remove trailing spaces in mailnews/base/search. rs=white-space-only

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fd85506c79b1
remove trailing spaces in mailnews/base/util. rs=white-space-only

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c1507b4ecc44
remove trailing spaces in mailnews/base. rs=white-space-only

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e08911b56e26
remove trailing spaces in mailnews/compose. rs=white-space-only

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2434e5fd8480
white-space clean-up in mailnews/compose/src/nsMsgAppleDouble.h. rs=white-space-only

Comment 6

[Jorg K (CEST = GMT+2)]

By accident this patch also included removal of trailing white-space in mailnews/db. Too bad, but not worth backing it out for that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8f431a95645c
remove trailing spaces in mailnews/news. rs=white-space-only

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c0e3022858b
remove trailing spaces in mailnews/local. rs=white-space-only CLOSED TREE

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/423b32636d1c
fix white-space in nsVCardObj.cpp and nsVCardObj.h. rs=white-space-only

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/eabd24878a58
remove trailing spaces in mailnews/addrbook. rs=white-space-only

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c51e042d239
replace CRLF EOL with LF. rs=white-space-only

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/de88484ec536
remove trailing spaces in mailnews/extensions. rs=white-space-only

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fbac67197f7b
remove trailing spaces in mailnews/intl. rs=white-space-only

Comment 14

[Jorg K (CEST = GMT+2)]

For the record: imap, import, mapi and mime are left.

Comment 15

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #14)
> For the record: imap, import, mapi and mime are left.

+ IDL files (bug 1389062 comment 30).

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8c5ae32e395e
remove trailing spaces in mailnews/mapi. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/12a18e736fca
remove trailing spaces from IDL files in mailnews. rs=white-space-only

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/25aad613d94b
remove trailing spaces from JS/JSM files in mail/. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/13506ae35338
remove trailing spaces from JS/JSM files in mailnews/. rs=white-space-only

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f66ed82ee22d
remove trailing spaces in mailnews/import. rs=white-space-only

Comment 19

[Jorg K (CEST = GMT+2)]

For the record: imap and mime are left.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/961aa136630d
remove trailing spaces in ldap. rs=white-space-only

Comment 21

[Frank-Rainer Grahl (:frg)]

> remove trailing spaces in ldap. rs=white-space-only

If you want a patch I can do one for the tabs per directory here.

Comment 22

[Jorg K (CEST = GMT+2)]

Yes, I can always use a patch when I have no other. You mean to convert tabs to spaces? That's very tricky, since at times tabs mean 2 spaces, other times 4. I've tried a few files with varying success. Do you have a script for this? Are there many tabs?

Comment 23

[Frank-Rainer Grahl (:frg)]

> Do you have a script for this? Are there many tabs?

ldap is full of them. I probably need to do it manually or might try to load them into eclipse and use its code formatter. They don't align at all. It is neither 2 nor 4...

Comment 24

[Frank-Rainer Grahl (:frg)]

Attachment: 1399756-tabs-ldap-include.patch

Just a simple replacement with Notepad++ and a few manual corrections. Over 1000 tabs and the formatting is ...

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951452 [details] [diff] [review]
1399756-tabs-ldap-include.patch

Hmm, I see trailing spaces in the patch, I thought I removed them all.

The problem is that things don't line up any more, please check it in the review preview. As you know, a tab will advance to the next tab stop, so it can represent 1 to N spaces.

Comment 26

[Jorg K (CEST = GMT+2)]

Oh, I guess the now trailing spaces were trailing tabs before :-(

Comment 27

[Frank-Rainer Grahl (:frg)]

Yes but they didn't line up before depending on your settings. Was just a quick proof of concept :) I better look for a code formatter.

Some files in mailnews and elsewhere also have tabs. Stumble over them now and then. Might be a good idea to get rid of them in the 60 cycle.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/af402e8367c3
remove trailing spaces in mailnews/imap. rs=white-space-only

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/30b555fe9c6e
remove trailing spaces in db (Mork). rs=white-space-only

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/18e13d30dbea
remove trailing spaces in mailnews/mime. rs=white-space-only

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4d1bcb81b2f6
remove trailing spaces in mailnews/ (.xul, .xml, .css, .mm, .cpp). rs=white-space-only
https://hg.mozilla.org/comm-central/rev/cb4a952a64c3
convert line endings in feedAccountWizard.xul. rs=white-space-only

Comment 32

[Jorg K (CEST = GMT+2)]

In bug 1446802 M-C used "codespell" (https://github.com/lucasdemarchi/codespell, install: pip install codespell) to fix some typos. There are heaps of typos in the C-C codebase as I found out.

codespell -w -q4 --skip="./.hg,mozilla,obj-x86_64-pc-mingw32,*.mab" --ignore-words=../../ignore.txt
seems to work OK, but the correction is very zealous and needs to be checked manually.

The words to ignore must include
aparent (aParent)
acount (aCount)
aline (aLine)
isnt
which would be changed to apparent, account, align and "isn't" which doesn't work well in nsMsgSearchOp.Isnt :-(
The tool also modifies code, so careful!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2dc101e7eddc
fix typos in db/ using codespell. rs=comment-only,typo-fix

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-calendar.patch

Philipp and MakeMyDay, are you interested to have spelling errors in Calendar fixed? Here's the patch that will do that.

Mostly comments, some incorrect strings and even some bugs where "command" is spelled "commmand", which I don't think will work.

Linting fans should also be spelling fans, no?

Comment 35

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-mail.patch

Plenty of spelling problems in mail/ as well. This also found a bug, "resizeable" won't work as intended.

Comment 36

[:aceman]

The same 'resizeable' bug is also in suite. Maybe we should do at least that one change for them.

Comment 37

[:aceman]

Comment on attachment 8960399 [details] [diff] [review]
1399756-mail.patch

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

Nice, thanks:)
But we should check with the IM guys if they really want the class name changed.

Comment 38

[:aceman]

Comment on attachment 8960395 [details] [diff] [review]
1399756-calendar.patch

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

::: calendar/providers/wcap/calWcapCalendarItems.js
@@ +1148,5 @@
>          request.execRespFunc(null, []);
>          return request;
>      }
>  
> +    // m_cachedResults holds the last data revtrieval. This is especially useful when

revtrieval? What has changed here?

Comment 39

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #37)
> But we should check with the IM guys if they really want the class name
> changed.
You're referring to the CSS class: collapsable -> collapsible.

(In reply to :aceman from comment #38)
> > +    // m_cachedResults holds the last data revtrieval. This is especially useful when
> revtrieval? What has changed here?
Good question. Don't blame me for codespell bugs, looks like this one needs to be fixed manually.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-mail.patch - some more

Comment 41

[:aceman]

Comment on attachment 8960487 [details] [diff] [review]
1399756-mail.patch - some more

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

Where did the 'revtrieval' fix go?

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-calendar.patch - some more

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-calendar.patch - some more + revtrieval

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-mail.patch - some more but without collapsable

collapsable is an alternate spelling of collapsible, so pulling this change out.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/37b26fb2a421
fix typos in mailnews/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/64fb777301cd
fix typos in mail/ using codespell. r=aceman

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-chat.patch

Comment 47

[Florian Quèze [:florian]]

Comment on attachment 8960527 [details] [diff] [review]
1399756-chat.patch

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

We are not the upstream for chat/protocols/matrix/matrix-sdk and chat/protocols/twitter/twitter-text.jsm. The other changes seem OK to me.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1399756-chat.patch (v2)

OK, removed chat/protocols/matrix/matrix-sdk/* and chat/protocols/twitter/twitter-text.jsm (still changing chat/protocols/twitter/twitter.js).

Comment 49

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 8960495 [details] [diff] [review]
1399756-calendar.patch - some more + revtrieval

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

Please exclude libical and ical.js as they are upstream libraries.

Given this was automated maybe we can postpone it to after landing the calutils patches, as I can imagine there will be some bitrot.

Out of curiosity, how does codespell handle typos in strings? I see it made a correction to Components.Exception("seting..."), but how does it know that this is a user-visible string as opposed to some identifier that cannot change?

Comment 50

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #49)
> Please exclude libical and ical.js as they are upstream libraries.
Will do.

> Given this was automated maybe we can postpone it to after landing the
> calutils patches, as I can imagine there will be some bitrot.
Sure, no problem.

> Out of curiosity, how does codespell handle typos in strings? I see it made
> a correction to Components.Exception("seting..."), but how does it know that
> this is a user-visible string as opposed to some identifier that cannot
> change?
You'd need to read the documentation: https://github.com/lucasdemarchi/codespell. It has a list of common miss-spellings, here's an excerpt:
abotu->about
abouta->about a
aboutit->about it
aboutthe->about the
abouve->above
abov->above

It is very zealous and *will* change variable names. Therefore you need to check the result. You also need an exceptions file that needs, for example "aCount" (see comment #32), since this would be changed to "account".

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0f8c4b23d368
fix typos in editor/ using codespell. rs=comment-only,typo-fix

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2daea21a0ede
remove trailing spaces in mail/ (.css, .dtd, .properties). rs=white-space-only

Comment 53

[Jorg K (CEST = GMT+2)]

Uplifted the typo corrections to avoid future merge problems:
https://hg.mozilla.org/releases/comm-beta/rev/502c294f53bab26443da3a4997629f23b70669a6 - db
https://hg.mozilla.org/releases/comm-beta/rev/e8f902aac0b0dc1ab894220b2280dec75d7799cc - mailnews
https://hg.mozilla.org/releases/comm-beta/rev/81e6edf209a93cc2078a57b5c61d792b9d04c999 - mail
https://hg.mozilla.org/releases/comm-beta/rev/c10714b39cbdb2f3b591c78e41a46a8dceb5b37e - editor

Comment 54

[Onno Ekker [:nONoNonO UTC+1]]

Attachment: Some more white space changes [landed in comment #58]

When comparing the changed white-space to my localized repository, I found some more changes in spacing of comments, missing eol on the last line of files, and wrong file modes.

Comment 55

[Onno Ekker [:nONoNonO UTC+1]]

Attachment: Remove trailing empty lines [landed in comment #58]

There are some .dtd and .properties files that have extra empty lines at the end of the file. This patch removes those empty lines.

Comment 56

[Jorg K (CEST = GMT+2)]

OK, thanks, I'll used these patched eventually.

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 8964816 [details] [diff] [review]
Some more white space changes [landed in comment #58]

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

::: mail/locales/en-US/chrome/messenger/extensionsOverlay.dtd
@@ +8,5 @@
> +<!-- LOCALIZATION NOTE: RTL languages may wish to switch these -->
> +<!ENTITY goBackCmd.keyCode "VK_LEFT">
> +<!ENTITY goForwardCmd.keyCode "VK_RIGHT">
> +<!ENTITY goBackCmd.commandKey "[">
> +<!ENTITY goForwardCmd.commandKey "]">

What happened here? Where do these come from?

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c75dc9554a4f
Remove some trailing empty lines in .properties and .dtd files in mail/. r=jorgk
https://hg.mozilla.org/comm-central/rev/5e66a21a7aba
Fix some white-space issues in .properties and .dtd files in mail/. r=jorgk

Comment 59

[Jorg K (CEST = GMT+2)]

Thanks Onno. I didn't land the hunk I mentioned in comment #57. Also, wrong file modes are not relevant on Windows, a "hg qref" removes those hunks, so I didn't land that either. Otherwise, much appreciated, I always need patches to trigger C-C builds after M-C merges.

Comment 60

[Jorg K (CEST = GMT+2)]

Florian, any chance to check those spelling errors again?

Comment 61

[Florian Quèze [:florian]]

Comment on attachment 8960533 [details] [diff] [review]
1399756-chat.patch (v2)

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

Sorry for the delay.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e84d388d2c6
fix typos in chat/ using codespell. r=florian

Comment 63

[Jorg K (CEST = GMT+2)]

Let's finish the bug off now and start another one.

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9852dfc15675
remove trailing empty lines in editor/. r=jorgk

Comment 65

[Jorg K (CEST = GMT+2)]

Comment on attachment 8951452 [details] [diff] [review]
1399756-tabs-ldap-include.patch

I'll take care of LDAP in bug 1463266.