As you may have guessed, I am a big fan of linting. For me it makes the
code pretty and if it really restricts meaning in the code then maybe
the rules are just too strict. There is a balance to be found though. I
am happy with the calendar rules as they are, but others may think they
are too strict.

Yes, patches will break, so I'd wait for a time where we don't have any
major patches outstanding. My suggestion would be to make lint changes
per rule and land as early as possible. You could even break it up more
(which is probably unavoidable) by doing per rule per
component/directory. With small changesets like this it is generally
easy to adapt patches that have bitrotted due to the lint changes.

I am not overly concerned about the backporting, we shouldn't be
backporting too much anyway and yes maybe there are 2-3 cycles where
backporting is a bit more work, but I think that is ok.

Philipp

On 7/27/17 2:07 AM, R Kent James via Maildev wrote:

Tom Prince via Maildev wrote on 27.07.2017 06:03:

No objection from me to use that for Calendar. If the codebase is
already almost in line, the changes should be minor there.

That is a good compromise, if you guys think that linting is good.

On 7/27/2017 4:49 AM, Ben Bucksch via Maildev wrote:

I don't think that is a good idea. It adds plenty of pain (having to
understand linting, and partially apply it) with very little gain.
Either the code is changed to have a consistent style, or it isn't.
Forcing new code to have a consistent style, which might be inconsistent
with an (unfixed) rest of the file, seems rather pointless to me. I'd
rather do nothing at all than that. Linting also catches some real bugs,
like unused variables, and if an editor generates zillions of errors
when linting is applied, you'll just ignore them instead of seeing if
any are real bugs.

I need opinions here about whether anybody else supports this idea, and
Tom you need to be clear if this is what you support or not. Just to be
clear, this approach means that you are against the major project that I
am proposing to add linting to existing /mailnews and /mail code.

:rkent

On Thu, Jul 27, 2017 at 11:35 AM R Kent James kent@caspia.com wrote:

If I was making the decision, I would turn on linting for all the code,
both C++ and JS. Given that I am now responsible for the build system, I *
am* going to turn it on for python.

Regarding incremental linting, I think that is only an option if there is
good tooling around it. In particular, I'd want tools that only show you
the lint errors that are in new and changed lines. And also only if the new
style is close enough to the old style that it isn't completely jarring to
have mixed style in a single file.

Given the limited resources of the project, getting good tooling in place
for doing things incrementally probably isn't worth it.

-- Tom

On 7/27/2017 12:35 PM, R Kent James via Maildev wrote:

In my opinion, linting should be enabled and enforced for all of our
codebase. However, we should also review the rules first to see what's
going to cause problems. For example, I am a big believer in 80-char
lines, but I'll often drop that rule for lines where there's no
comfortable line break (hi, nsCOMPtr<nsIMsgSomeLongInterface> something
= do_GetService(NS_MSGSOMEGODAWFULTHING_CONTRACTID); and similar
things), and some things want tabular alignment involving what would
normally be extraneous indentation.

We should also be more willing to accept whitespace changes. It's really
annoying when you read a file where each nesting of an if statement is
using a different indentwidth parameter.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

On 7/27/17 2:55 AM, Enrico Weigelt, metux IT consult via Maildev wrote:

We did that for years in calendar not only for whitespaces but also for
things like var/let. We never touched enough lines to fix everything
consistently, and it was a mess.

Philipp

Yes, seems like fixing only changed lines is a bad idea.

I see no benefit in doing it piecemeal, e.g. file by file, either. It just prolongs the pain.  I'd rather have one version after which i have to merge everything.

If git can automatically merge whitespace, then the bitrot is not a problem,  either.  There's an git merge -Xignore-whitespace or similar option, but i don't know how that reacts in more complex situations.

Finally,  the lint rules would need to leave enough breathing room.

Ben

Am 27. Juli 2017 02:55:16 MESZ schrieb "Enrico Weigelt, metux IT consult via Maildev" maildev@lists.thunderbird.net:

--
This email was sent from my phone.  Please excuse the brevity

Ben Bucksch wrote on 27.07.2017 23:55:

git merge -Xignore-all-space

        ignore-space-change, ignore-all-space, ignore-space-at-eol
            Treats lines with the indicated type of whitespace change as
            unchanged for the sake of a three-way merge. Whitespace 

changes
mixed with other changes to a line are not ignored. See also
git-diff(1) -b, -w, and --ignore-space-at-eol.

            ·   If their version only introduces whitespace changes to a
                line, our version is used;

            ·   If our version introduces whitespace changes but their
                version includes a substantial change, their version is
                used;

            ·   Otherwise, the merge proceeds in the usual way.

On 7/27/17 11:55 PM, Ben Bucksch via Maildev wrote:

The benefit is that regressions are easier to bisect and that the
patches don't bitrot while writing them. Fixing all style in Thunderbird
is going to be a massive undertaking. mail/components alone has 26492
errors if you use the calendar/ rules (which may obviously not be always
suitable, but just as a ballpark figure).

If you split up commits by component and rule, then regressions can
easily be found, and working patches can be pushed earlier.

Something different, with above mentioned volume of changes, it is
questionable if it is really worth the effort if we are going to be
replacing components with newly rewritten code.

Philipp

On 7/30/2017 4:44 PM, Philipp Kewisch via Maildev wrote:

One issue I have seen consistently as a barrier to progress within
Mozilla is ongoing threats that such-and-such code is going to be
obsolete Real Soon Now, so we stop trying to fix issues with it or
improve it. A great example is a freeze many years ago on new XUL
features, that was mostly done to reject a patch that Thunderbird wanted
that would have supported multiple lines in trees. XUL is still with us,
with no active efforts ongoing to change that. Now this TB:NG discussion
is threatening to prematurely freeze improvements in all of Thunderbird.
Let's be careful before we stop doing improvements in existing code
based on unresolved discussions about a possible future.

Yes we need to have that TB:NG discussion, and I hope to make some
proposals along that line here soon. (And for the record, I've proposed
in conjunction with TB:NG that we set a sunset date for existing code,
for which I did not receive much support). But at this point in time,
TB:NG is not policy, it is a proposal that has many, many preliminaries
that must be accomplished before it could be the plan, even if that is
the direction that we wanted to go. For now, let's please not use that
discussion as a reason to stop improving aspects of the existing code.
Yes let's plan as part of TB:NG if and when to reduce efforts on the
existing codebase , but reasonably the chances of a TB:NG rewrite from
scratch ever happening are pretty low at the moment, so let' not plan as
if that is our future.

Concerning the general proposal to start doing linting of existing
Thunderbird mail/ and mailnews/ code, I think we have had enough
discussions now to say that there is a rough consensus to support a
volunteer team doing this. Remaining details can be discussed in bugs
that are filed as part of this.

:rkent