| 1 |
On 04/18/2016 01:14 PM, Adam Mills wrote: |
| 2 |
> Thanks for all the feedback so far. I probably should have just filed |
| 3 |
> the bug |
| 4 |
> and left it at that, but I'm committed now. |
| 5 |
> |
| 6 |
> I've done my best to incorporate all of the suggestions here, and |
| 7 |
> submitted a |
| 8 |
> new patch rev 3. I have a few questions about details I'm pretty sure still |
| 9 |
> need improvement: |
| 10 |
> |
| 11 |
> 1) In order to build the InternalPackageSet only once, I used the |
| 12 |
> _DisplayConfig target_root, instead of using pkg.root in the check_sets |
| 13 |
> method. I'm not sure of the pkg.root purpose, but if there can be |
| 14 |
> different |
| 15 |
> roots, the InternalPackageSet would have to be created for each package. |
| 16 |
> If that's the case, it could be moved to the check_sets method, and |
| 17 |
> created |
| 18 |
> for each package. |
| 19 |
|
| 20 |
You should create the InternalPackageSet for each root, since each root |
| 21 |
has its own package sets. |
| 22 |
|
| 23 |
> 2) I added the new InternalPackageSet as a new member of the _DisplayConfig |
| 24 |
> class. Let me know if there's somewhere more appropriate, especially |
| 25 |
> after |
| 26 |
> the feedback from #1. |
| 27 |
|
| 28 |
I guess that's fine, since we're not using it anywhere else. |
| 29 |
|
| 30 |
> 3) Is there a better method available for merging PackageSets? |
| 31 |
|
| 32 |
Yes, you should add all of the atoms to it at once, because each update |
| 33 |
calls _updateAtomMap which is expensive. For example, use something like |
| 34 |
this: |
| 35 |
|
| 36 |
user_sets = |
| 37 |
InternalPackageSet(initial_atoms=itertools.chain.from_iterable(pkgset.getAtoms() |
| 38 |
for pkgset in pkgsets if pkgset.user_set)) |
| 39 |
-- |
| 40 |
Thanks, |
| 41 |
Zac |
Archives