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

RESOLVED FIXED in Thunderbird 61.0

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jorgk, Unassigned)

Tracking

Trunk
Thunderbird 61.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

Reporter

Description

2 years ago
+++ 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.
Reporter

Updated

2 years ago
Keywords: leave-open

Comment 1

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/24067b756994
remove trailing spaces in mailnews/base/search. rs=white-space-only

Comment 2

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd85506c79b1
remove trailing spaces in mailnews/base/util. rs=white-space-only

Comment 3

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c1507b4ecc44
remove trailing spaces in mailnews/base. rs=white-space-only

Comment 4

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e08911b56e26
remove trailing spaces in mailnews/compose. rs=white-space-only

Comment 5

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2434e5fd8480
white-space clean-up in mailnews/compose/src/nsMsgAppleDouble.h. rs=white-space-only
Reporter

Comment 6

2 years ago
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

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f431a95645c
remove trailing spaces in mailnews/news. rs=white-space-only

Comment 8

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9c0e3022858b
remove trailing spaces in mailnews/local. rs=white-space-only CLOSED TREE

Comment 9

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/423b32636d1c
fix white-space in nsVCardObj.cpp and nsVCardObj.h. rs=white-space-only

Comment 10

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eabd24878a58
remove trailing spaces in mailnews/addrbook. rs=white-space-only

Comment 11

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9c51e042d239
replace CRLF EOL with LF. rs=white-space-only

Comment 12

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/de88484ec536
remove trailing spaces in mailnews/extensions. rs=white-space-only

Comment 13

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fbac67197f7b
remove trailing spaces in mailnews/intl. rs=white-space-only
Reporter

Comment 14

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

Comment 15

2 years ago
(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

2 years ago
Pushed by mozilla@jorgk.com:
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

Last year
Pushed by mozilla@jorgk.com:
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f66ed82ee22d
remove trailing spaces in mailnews/import. rs=white-space-only
Reporter

Comment 19

Last year
For the record: imap and mime are left.

Comment 20

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/961aa136630d
remove trailing spaces in ldap. rs=white-space-only
> remove trailing spaces in ldap. rs=white-space-only

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

Comment 22

Last year
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?
> 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...
Just a simple replacement with Notepad++ and a few manual corrections. Over 1000 tabs and the formatting is ...
Reporter

Comment 25

Last year
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.
Reporter

Comment 26

Last year
Oh, I guess the now trailing spaces were trailing tabs before :-(
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/af402e8367c3
remove trailing spaces in mailnews/imap. rs=white-space-only

Comment 29

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/30b555fe9c6e
remove trailing spaces in db (Mork). rs=white-space-only

Comment 30

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/18e13d30dbea
remove trailing spaces in mailnews/mime. rs=white-space-only

Comment 31

Last year
Pushed by mozilla@jorgk.com:
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
Reporter

Comment 32

Last year
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2dc101e7eddc
fix typos in db/ using codespell. rs=comment-only,typo-fix
Reporter

Comment 34

Last year
Posted patch 1399756-calendar.patch (obsolete) — Splinter Review
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?
Attachment #8960395 - Flags: review?(philipp)
Attachment #8960395 - Flags: review?(makemyday)
Reporter

Comment 35

Last year
Posted patch 1399756-mail.patch (obsolete) — Splinter Review
Plenty of spelling problems in mail/ as well. This also found a bug, "resizeable" won't work as intended.
Attachment #8960399 - Flags: review?(acelists)

Comment 36

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

Comment 37

Last year
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.
Attachment #8960399 - Flags: review?(florian)

Comment 38

Last year
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?
Reporter

Comment 39

Last year
(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.
Reporter

Comment 40

Last year
Attachment #8960399 - Attachment is obsolete: true
Attachment #8960399 - Flags: review?(florian)
Attachment #8960399 - Flags: review?(acelists)
Attachment #8960487 - Flags: review?(florian)
Attachment #8960487 - Flags: review?(acelists)

Comment 41

Last year
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?
Attachment #8960487 - Flags: review?(acelists) → review+
Reporter

Comment 42

Last year
Attachment #8960395 - Attachment is obsolete: true
Attachment #8960395 - Flags: review?(philipp)
Attachment #8960395 - Flags: review?(makemyday)
Attachment #8960493 - Flags: review?(philipp)
Attachment #8960493 - Flags: review?(makemyday)
Reporter

Comment 43

Last year
Attachment #8960493 - Attachment is obsolete: true
Attachment #8960493 - Flags: review?(philipp)
Attachment #8960493 - Flags: review?(makemyday)
Attachment #8960495 - Flags: review?(philipp)
Attachment #8960495 - Flags: review?(makemyday)
Reporter

Comment 44

Last year
collapsable is an alternate spelling of collapsible, so pulling this change out.
Attachment #8960487 - Attachment is obsolete: true
Attachment #8960487 - Flags: review?(florian)
Attachment #8960517 - Flags: review+

Comment 45

Last year
Pushed by mozilla@jorgk.com:
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
Reporter

Comment 46

Last year
Posted patch 1399756-chat.patch (obsolete) — Splinter Review
Attachment #8960527 - Flags: review?(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.
Attachment #8960527 - Flags: review?(florian) → review-
Reporter

Comment 48

Last year
OK, removed chat/protocols/matrix/matrix-sdk/* and chat/protocols/twitter/twitter-text.jsm (still changing chat/protocols/twitter/twitter.js).
Attachment #8960527 - Attachment is obsolete: true
Attachment #8960533 - Flags: review?(florian)
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?
Attachment #8960495 - Flags: review?(philipp)
Attachment #8960495 - Flags: review?(makemyday)
Attachment #8960495 - Flags: feedback+
Reporter

Comment 50

Last year
(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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0f8c4b23d368
fix typos in editor/ using codespell. rs=comment-only,typo-fix

Comment 52

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2daea21a0ede
remove trailing spaces in mail/ (.css, .dtd, .properties). rs=white-space-only
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.
There are some .dtd and .properties files that have extra empty lines at the end of the file. This patch removes those empty lines.
Reporter

Comment 56

Last year
OK, thanks, I'll used these patched eventually.
Reporter

Comment 57

Last year
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

Last year
Pushed by mozilla@jorgk.com:
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
Reporter

Comment 59

Last year
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.
Reporter

Updated

Last year
Attachment #8964816 - Attachment description: Some more white space changes → Some more white space changes [landed in comment #58]
Reporter

Updated

Last year
Attachment #8964817 - Attachment description: Remove trailing empty lines → Remove trailing empty lines [landed in comment #58]
Reporter

Comment 60

Last year
Florian, any chance to check those spelling errors again?
Flags: needinfo?(florian)
Comment on attachment 8960533 [details] [diff] [review]
1399756-chat.patch (v2)

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

Sorry for the delay.
Attachment #8960533 - Flags: review?(florian) → review+
Flags: needinfo?(florian)

Comment 62

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2e84d388d2c6
fix typos in chat/ using codespell. r=florian
Reporter

Comment 63

Last year
Let's finish the bug off now and start another one.
Keywords: leave-open

Comment 64

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9852dfc15675
remove trailing empty lines in editor/. r=jorgk
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Reporter

Updated

Last year
Target Milestone: --- → Thunderbird 61.0
Reporter

Comment 65

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

I'll take care of LDAP in bug 1463266.
Attachment #8951452 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.