1 |
On Sun, 24 Apr 2016 15:04:57 -0700 |
2 |
Zac Medico <zmedico@g.o> wrote: |
3 |
|
4 |
> On 04/24/2016 02:45 PM, Zac Medico wrote: |
5 |
> > On 04/24/2016 01:00 AM, Brian Dolbec wrote: |
6 |
> >> On Sun, 24 Apr 2016 08:15:56 +0200 |
7 |
> >> Michał Górny <mgorny@g.o> wrote: |
8 |
> >> |
9 |
> >>> On Sat, 23 Apr 2016 16:57:21 -0700 |
10 |
> >>> Zac Medico <zmedico@g.o> wrote: |
11 |
> >>> |
12 |
> >>>> Fix ArchChecks to not mix arches of ebuilds together, so that |
13 |
> >>>> errors are only reported for those arches that the ebuild has |
14 |
> >>>> keywords for. |
15 |
> >>>> --- |
16 |
> >>>> Applies to the *repoman* branch. |
17 |
> >>>> |
18 |
> >>>> pym/repoman/modules/scan/arches/arches.py | 2 +- |
19 |
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-) |
20 |
> >>>> |
21 |
> >>>> diff --git a/pym/repoman/modules/scan/arches/arches.py |
22 |
> >>>> b/pym/repoman/modules/scan/arches/arches.py index |
23 |
> >>>> 4df25a8..6e1c17d 100644 --- |
24 |
> >>>> a/pym/repoman/modules/scan/arches/arches.py +++ |
25 |
> >>>> b/pym/repoman/modules/scan/arches/arches.py @@ -69,7 +69,7 @@ |
26 |
> >>>> class ArchChecks(ScanBase): arches.add(('**', '**', ('**',))) |
27 |
> >>>> # update the dynamic data |
28 |
> >>>> dyn_arches = kwargs.get('arches') |
29 |
> >>>> - #dyn_arches.clear() |
30 |
> >>>> + dyn_arches.clear() |
31 |
> >>>> dyn_arches.update(arches) |
32 |
> >>> |
33 |
> >>> Is that some crazy way of replacing contents of an existing dict? |
34 |
> >>> May be worth a comment since the code looks suspicious, at least. |
35 |
> >>> |
36 |
> >> |
37 |
> >> It was an interim step so that modules didn't pass back |
38 |
> >> arbitrary data to be re-used by other modules after it. This way |
39 |
> >> maintains the data pointer to the set (not dict) object in the |
40 |
> >> calling function. Just assigning it a new set doesn't set the |
41 |
> >> 'arches' pointer in the calling function so that it can pass that |
42 |
> >> data along to other modules. |
43 |
> > |
44 |
> > I feel like the plugin structure has lead to decoupling of things |
45 |
> > that really should be coupled. For example, the code in the |
46 |
> > ArchChecks could be a function that's called by the |
47 |
> > ProfileDependsChecks plugin, given that ProfileDependsChecks seems |
48 |
> > to be the only consumer of this data. |
49 |
> |
50 |
> We'll have to go through all of the plugins and look for other things |
51 |
> that have too aggressively decoupled. The fact Scanner._scan_ebuilds |
52 |
> calls plugins in a hardcoded order is troubling. It means that there |
53 |
> may be coupling between plugins that makes the order matter, yet this |
54 |
> coupling is hidden and non-trivial to discover. If we couple more |
55 |
> things together where appropriate (like ArchChecks and |
56 |
> ProfileDependsChecks), then it will make the system easier for a |
57 |
> person to reason about. |
58 |
|
59 |
Yes, I agree, but the split out process was a long tedious one. And in |
60 |
making this module system, I did them in the order they were run |
61 |
originally. The code was split up all over with more and more things |
62 |
added to the spaghetti code that ended up at 1000+ lines of code for |
63 |
the one pkg & ebuild loop. It's no wonder things got split badly. |
64 |
|
65 |
The code now can be given decent scrutiny to optimize and refactor |
66 |
where appropriate. Like re-coupling some split up code. |
67 |
|
68 |
-- |
69 |
Brian Dolbec <dolsen> |