[Yum-devel] Updated repository/Next refactoring steps/Request for review

[Florian Festi]

James Bowes wrote:
> So I played around with the ffesti branch/patch series tonight, and
> everything looks good to me. I'd say this is ok to merge, with the
> exception of the sqlite index workaround.
Yeah, I just put that in to avoid patching the metadata-parser all the time. 
Nevertheless we can discuss if it makes sense to create all indexes on the 
client side. Creating them is cheap (as long as you don't have an XO) and it 
should save a significant percentage download size. But I don't have all 
numbers at hand right now and we can consider it after we got the patches 
merged. If we get the delta meta data code working this considerations are 
obsolete anyway...

> That said, I have a few minor nits, some of which are with the code
> surrounding your changes, rather than your changes themselves. None of
> these should stop your patches from being merged, IMO:
I tried to keep the size of the patch below 1000 lines so I couldn't do too 
much additional cosmetics ;)=

>  * There are some decent sized chunks of commented out code that should
>    just be removed.
>  * We should replace the 'return 0' and 'return 1' with real booleans.
>  * It would be nice to have better names for get(Old|New)Provides and
>    Requires (I can't think of any myself, however).
Yeah, better name is welcome. If I'd know one myself we wouldn't discuss 
that right now...
>  * Likewise for tsInfo.setDatabases (maybe make this two calls?)
I don't think it makes sense to set just one. A nicer interface would be to 
pass the databases to the constructor but I wanted to avoid the interface 
change to increase the change of getting the patches merged. Fell free to 
adjust the code.

>  * 3 of the depsolve unit tests fail, but only because the fake rpmdb is
>    missing the new calls.
Ehmm, ok, I should have caught that one myself. Why don't we use a 
PackageSack for this?

> Nice work.
Thanks!

Thanks for taking care!

Florian