1 |
On Fri, 2018-11-02 at 01:27 +0300, Andrew Savchenko wrote: |
2 |
> Hi! |
3 |
> |
4 |
> On Tue, 30 Oct 2018 08:18:58 +0100 Michał Górny wrote: |
5 |
> > On Mon, 2018-10-29 at 03:57 +0300, Andrew Savchenko wrote: |
6 |
> > > On Sun, 28 Oct 2018 19:29:28 +0100 Michał Górny wrote: |
7 |
> > > > On Sun, 2018-10-28 at 01:38 +0300, Andrew Savchenko wrote: |
8 |
> > > > > Hi all! |
9 |
> > > > > |
10 |
> > > > > The only blocker for EAPI 7 update is eutils inheritance, but it |
11 |
> > > > > seems to be not used within the current eclass code, probably a |
12 |
> > > > > remnant from older days. So it is removed. |
13 |
> > > > > |
14 |
> > > > > Looks like no other EAPI 7 specific changes needed. |
15 |
> > > > > |
16 |
> > > > |
17 |
> > > > Please use -U99999 to include more context to the patches. |
18 |
> > |
19 |
> > I'm going to include a few 'easy cleanup' comments since EAPI 7 |
20 |
> > is a good opportunity to improve the eclass. I'm going to skip horribly |
21 |
> > bad design decisions since I suppose nobody cares. |
22 |
> |
23 |
> Should we really mix EAPI bump with full code review? |
24 |
|
25 |
Yes, for two reasons. |
26 |
|
27 |
Firstly, because an EAPI bump effectively requires reviewing all |
28 |
the eclass logic for constraints imposed by the new EAPI. While |
29 |
reviewing code, it is natural that people may notice other issues. |
30 |
Ignoring them once noticed would be a waste of effort. |
31 |
|
32 |
Secondly, changes to frequently used eclass have a large overhead of |
33 |
metadata cache updates. Given most of the listed issues are rather |
34 |
trivial to fix, it would be wasteful to defer them for a second metadata |
35 |
cache update. |
36 |
|
37 |
> This eclass is small, so no harm here. But for larger eclasses |
38 |
> (hello java-*.eclass) this will hinder updates considerably. I |
39 |
> prefer to fix something rather than to fix nothing while |
40 |
> frustrating in attempt to fix everything at once. |
41 |
> |
42 |
> Also this make git history review harder as fixes for independent |
43 |
> issues will be mixed together. |
44 |
|
45 |
Why would you mix them together? The whole point of using git (and not |
46 |
CVS) is that you can trivially make separate commits addressing |
47 |
different kinds of issues. It also makes it trivial to send them for |
48 |
review afterwards. |
49 |
|
50 |
> So I kindly ask you for future updates (from everyone, not just |
51 |
> me) focus on review of the proposed changes instead of reviewing |
52 |
> full code. Thank you for understanding. |
53 |
|
54 |
As explained above, the proposed change is meaningless without context |
55 |
(as it affects how everything else in eclass works). If we were to |
56 |
ignore context, we'd even ACK eclass changes that resulted in the eclass |
57 |
immediately dying due to programmer's mistake. |
58 |
|
59 |
Finally, I'd like to point out that peer review is one of foundations |
60 |
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 |
61 |
makes it even worse is that some developers are actively hostile to |
62 |
the criticism of their code. It is as if Gentoo's bazaar was dominated by makeshift cathedrals. |
63 |
|
64 |
-- |
65 |
Best regards, |
66 |
Michał Górny |