On Fri, 2018-11-02 at 01:27 +0300, Andrew Savchenko wrote:
> Hi!
> 
> On Tue, 30 Oct 2018 08:18:58 +0100 Michał Górny wrote:
> > On Mon, 2018-10-29 at 03:57 +0300, Andrew Savchenko wrote:
> > > On Sun, 28 Oct 2018 19:29:28 +0100 Michał Górny wrote:
> > > > On Sun, 2018-10-28 at 01:38 +0300, Andrew Savchenko wrote:
> > > > > Hi all!
> > > > > 
> > > > > The only blocker for EAPI 7 update is eutils inheritance, but it
> > > > > seems to be not used within the current eclass code, probably a
> > > > > remnant from older days. So it is removed.
> > > > > 
> > > > > Looks like no other EAPI 7 specific changes needed.
> > > > > 
> > > > 
> > > > Please use -U99999 to include more context to the patches.
> > 
> > I'm going to include a few 'easy cleanup' comments since EAPI 7
> > is a good opportunity to improve the eclass.  I'm going to skip horribly
> > bad design decisions since I suppose nobody cares.
> 
> Should we really mix EAPI bump with full code review?

Yes, for two reasons.

Firstly, because an EAPI bump effectively requires reviewing all
the eclass logic for constraints imposed by the new EAPI.  While
reviewing code, it is natural that people may notice other issues. 
Ignoring them once noticed would be a waste of effort.

Secondly, changes to frequently used eclass have a large overhead of
metadata cache updates.  Given most of the listed issues are rather
trivial to fix, it would be wasteful to defer them for a second metadata
cache update.

> This eclass is small, so no harm here. But for larger eclasses
> (hello java-*.eclass) this will hinder updates considerably. I
> prefer to fix something rather than to fix nothing while
> frustrating in attempt to fix everything at once.
> 
> Also this make git history review harder as fixes for independent
> issues will be mixed together.

Why would you mix them together?  The whole point of using git (and not
CVS) is that you can trivially make separate commits addressing
different kinds of issues.  It also makes it trivial to send them for
review afterwards.

> So I kindly ask you for future updates (from everyone, not just
> me) focus on review of the proposed changes instead of reviewing
> full code. Thank you for understanding.

As explained above, the proposed change is meaningless without context
(as it affects how everything else in eclass works).  If we were to
ignore context, we'd even ACK eclass changes that resulted in the eclass
immediately dying due to programmer's mistake.

Finally, I'd like to point out that peer review is one of foundations
of open source.  Sadly, Gentoo has failed to embrace this, and right now reviews of existing code are rather an exception than a rule.  What
makes it even worse is that some developers are actively hostile to
the criticism of their code.  It is as if Gentoo's bazaar was dominated by makeshift cathedrals.

-- 
Best regards,
Michał Górny