[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lmi] Code review: product editor

From: Evgeniy Tarassov
Subject: Re: [lmi] Code review: product editor
Date: Mon, 26 Mar 2007 15:48:28 +0200

On 3/26/07, Greg Chicares <address@hidden> wrote:
On 2007-3-21 17:36 UTC, Evgeniy Tarassov wrote:
This has four major parts:
(1) Rename and move DatabaseTreeItemData from a header to a TU
(2) DatabaseView::SetupControls() changes
(3) Addition of DatabaseView::table_adapter()
(4) DatabaseView::UponTreeSelectionChange() changes
I've tried to approach them in that order, which generally
follows the interdependencies.


(4) DatabaseView::UponTreeSelectionChange() changes

Could we lose the extra spaces please?
    LMI_ASSERT( item_id.IsOk() );
               ^              ^
    LMI_ASSERT( data );
And in (1), too:
    std::size_t id() const { return id_; }
    std::string const& description() const { return description_; }

Yes, sorry.

Maybe you want to write your own script to catch things like
that? We don't want the 'check_concinnity' makefile target to
grow much slower, and I've probably tuned it to catch only what
I've habitually overlooked myself.

What should I do about this? I don't want to commit to HEAD with
those spaces. I can remove them when committing to HEAD, but that
means that HEAD will diverge increasingly from the branch (as in
other examples above). Is that a problem? If it is, are we sure
we can manage it? Or is it exactly what we want?

I hope it never happen again, but this time it should not be a problem
if you commit the changed version to head. I'll do the same in the

AFAIU the current product editor branch is supposed to be disposed of
as soon as the changes are applied to the HEAD.

Maybe we could simplify the process this way:
1) I send you some patches and do commits to a newly created branch
2) you revise the patches and decide what patches should be fixed or
rewritten (like assigning accept/reject status for every patch)
3) the branch is disposed of
4) a new branch is created and I start fixing patches in the new branch
5) repeat the cycle

This way we avoid the branch and HEAD being too different after couple
of weeks. The cycle could take a week or be even shorter.

At any rate, I'm not going to try committing this part of the
patch yet, because it depends on other parts, and I'm trying to
do this one step at a time, making sure that I'm not introducing
a defect at any step. In the future, I think it'd save me a great
deal of time if I could ask you to make a separate patch for any
major renaming like (1)(c) above. (Does the "tier_fix_crash"
patch do anything like that?)

Yes, it does. Please, let me split it into smaller parts and send it
to you. I will do it shortly.

With best wishes,
Evgeniy Tarassov

reply via email to

[Prev in Thread] Current Thread [Next in Thread]