[PATCH 4 of 7] changeset: draft comment on changesets

[Mads Kiilerich]

On 04/24/2015 11:47 PM, Jan Heylen wrote:
> On Fri, Apr 24, 2015 at 7:21 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
>> On 04/14/2015 06:03 PM, Jan Heylen wrote:
>>> # HG changeset patch
>>> # User Jan Heylen <heyleke at gmail.com>
>>> # Date 1427526729 -3600
>>> #      Sat Mar 28 08:12:09 2015 +0100
>>> # Node ID f26b18234a7731a361f75e7a68ad143fc78218bf
>>> # Parent  f48bc03627761dd62f7e7094bb0d322834da336c
>>> changeset: draft comment on changesets
>>
>> This doesn't include any template changes at all so I doubt that description
>> is exact?
> Changeset is a controller? I'll add "(model)" in the commit message?

I like it when a commit message also summarizes what the changeset does 
so I can verify that it really does what it was intended to do ... 
especially when we don't have any tests that shows what changes. Ideally 
so that someone else based on that description could re-implement pretty 
much the same thing. But we should also be pragmatic ...

>>> +            for drafts in lines.values():
>>> +                c.draft_cnt += len(drafts)
>>
>> This extra iteration is inefficient. Let's hope it doesn't touch unloaded
>> objects where it would have to touch the db for each iteration.
>> Unfortunately that is how we do it ...
> comment_cnt is also there, Is that not needed neither?

What? I don't get that.

>> Please also consider adding test coverage for this new functionality.
> Yes, I consider this, but I first wanted to get a feeling (and I still
> do) on how feasible it is to get this patch series upstream (after
> adding tests).

Yes, I'm sure we can get something like this upstreamed. I just want to 
make sure we only add stuff that everybody do (or should) consider an 
improvement, moves in the right direction and keep reducing technical debt.

/Mads