Re: [lmi] Code review: product editor

[Evgeniy Tarassov]

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
branch.

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