Closed
Bug 1399756
Opened 7 years ago
Closed 6 years ago
Code clean-up: Style nits, typos and trailing spaces - take 2
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 61.0
People
(Reporter: jorgk-bmo, Unassigned)
Details
Attachments
(5 files, 6 obsolete files)
152.28 KB,
patch
|
Fallen
:
feedback+
|
Details | Diff | Splinter Review |
128.43 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
19.04 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
7.93 KB,
patch
|
Details | Diff | Splinter Review | |
8.22 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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•7 years ago
|
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
Reporter | ||
Comment 6•7 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.
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
Comment 10•7 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•7 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•6 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•6 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•6 years ago
|
||
For the record: imap, import, mapi and mime are left.
Comment 15•6 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•6 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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
For the record: imap and mime are left.
Comment 20•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/961aa136630d remove trailing spaces in ldap. rs=white-space-only
Comment 21•6 years ago
|
||
> 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•6 years ago
|
||
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•6 years ago
|
||
> 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•6 years ago
|
||
Just a simple replacement with Notepad++ and a few manual corrections. Over 1000 tabs and the formatting is ...
Reporter | ||
Comment 25•6 years ago
|
||
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•6 years ago
|
||
Oh, I guess the now trailing spaces were trailing tabs before :-(
Comment 27•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
The same 'resizeable' bug is also in suite. Maybe we should do at least that one change for them.
Comment 37•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Attachment #8960527 -
Flags: review?(florian)
Comment 47•6 years ago
|
||
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•6 years ago
|
||
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 49•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
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
Reporter | ||
Comment 53•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
OK, thanks, I'll used these patched eventually.
Reporter | ||
Comment 57•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
Attachment #8964816 -
Attachment description: Some more white space changes → Some more white space changes [landed in comment #58]
Reporter | ||
Updated•6 years ago
|
Attachment #8964817 -
Attachment description: Remove trailing empty lines → Remove trailing empty lines [landed in comment #58]
Reporter | ||
Comment 60•6 years ago
|
||
Florian, any chance to check those spelling errors again?
Flags: needinfo?(florian)
Comment 61•6 years ago
|
||
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+
Updated•6 years ago
|
Flags: needinfo?(florian)
Comment 62•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/2e84d388d2c6 fix typos in chat/ using codespell. r=florian
Reporter | ||
Comment 63•6 years ago
|
||
Let's finish the bug off now and start another one.
Keywords: leave-open
Comment 64•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•6 years ago
|
Target Milestone: --- → Thunderbird 61.0
Reporter | ||
Comment 65•6 years ago
|
||
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.
Description
•