Closed Bug 1399756 Opened 3 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jorgk-bmo, Unassigned)

Details

Attachments

(5 files, 6 obsolete files)

+++ 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.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/24067b756994
remove trailing spaces in mailnews/base/search. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd85506c79b1
remove trailing spaces in mailnews/base/util. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c1507b4ecc44
remove trailing spaces in mailnews/base. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e08911b56e26
remove trailing spaces in mailnews/compose. rs=white-space-only
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
By accident this patch also included removal of trailing white-space in mailnews/db. Too bad, but not worth backing it out for that.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8f431a95645c
remove trailing spaces in mailnews/news. rs=white-space-only
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
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
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/eabd24878a58
remove trailing spaces in mailnews/addrbook. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9c51e042d239
replace CRLF EOL with LF. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/de88484ec536
remove trailing spaces in mailnews/extensions. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fbac67197f7b
remove trailing spaces in mailnews/intl. rs=white-space-only
For the record: imap, import, mapi and mime are left.
(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).
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
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
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f66ed82ee22d
remove trailing spaces in mailnews/import. rs=white-space-only
For the record: imap and mime are left.
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.
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...
Attached patch 1399756-tabs-ldap-include.patch (obsolete) — Splinter Review
Just a simple replacement with Notepad++ and a few manual corrections. Over 1000 tabs and the formatting is ...
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.
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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/af402e8367c3
remove trailing spaces in mailnews/imap. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/30b555fe9c6e
remove trailing spaces in db (Mork). rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/18e13d30dbea
remove trailing spaces in mailnews/mime. rs=white-space-only
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
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!
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2dc101e7eddc
fix typos in db/ using codespell. rs=comment-only,typo-fix
Attached 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)
Attached 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)
The same 'resizeable' bug is also in suite. Maybe we should do at least that one change for them.
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 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?
(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.
Attached patch 1399756-mail.patch - some more (obsolete) — Splinter Review
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 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+
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)
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)
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+
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
Attached 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-
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+
(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".
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0f8c4b23d368
fix typos in editor/ using codespell. rs=comment-only,typo-fix
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.
OK, thanks, I'll used these patched eventually.
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?
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
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.
Attachment #8964816 - Attachment description: Some more white space changes → Some more white space changes [landed in comment #58]
Attachment #8964817 - Attachment description: Remove trailing empty lines → Remove trailing empty lines [landed in comment #58]
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)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2e84d388d2c6
fix typos in chat/ using codespell. r=florian
Let's finish the bug off now and start another one.
Keywords: leave-open
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
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.