Code review comment for lp:~jjed/archive-crawler/use-aptfile

On Tue, May 31, 2011 at 05:55:57AM -0000, Jacob Johan Edwards wrote:
> Jacob Johan Edwards has proposed merging lp:~j-johan-edwards/archive-crawler/use-aptfile into lp:~mvo/archive-crawler/mvo.
>
> Requested reviews:
> Michael Vogt (mvo)
>
> For more details, see:
> https://code.launchpad.net/~j-johan-edwards/archive-crawler/use-aptfile/+merge/62944
>
> Apologies for the huge patch. I abstracted IconFinder out, and that
> ballooned the line count of this solution quite a bit.

That is fine, the content is absolutely worth it!

> SUMMARY
>
> This branch adds a new component, `IconFinder`, which should solve the
> issue of finding icons in an archive. It is a frontend for `ArchiveCache`,
> a general solution for searching archive files via Contents-<ARCH>.gz.
>
> In the future, `ArchiveCache` might replace `ArchiveCrawler` as a cleaner,
> quicker, and more modularity-friendly solution. As it stands, only `IconFinder` uses it.

Awsome, this is great work already and has even more potential!

[..]
> TESTING
>
> Out of 361 desktop entries I tested (an old main[a-k] + universe[2-d]) with
> 343 icon requests, 13 failed. Of these, 11 failed because they requested
> non-existent icons, 1 failed because it requested an icon too large ('arista'),
> and 1 failed because it stored its icon in /usr/lib without a symlink in
> /usr/share or and absolute path request ('batmon.app').
>
> The unit tests that pass as of r123 pass now, as well as new ones for
> `IconFinder` and `ArchiveCache`.

Thanks for updating the tests as well (and adding more!), it looks
really well done.

> FUTURE
>
> Hopefully this branch -- together with a solution for extracting large icons
> -- will solve #599535. With the exception of arista, all icons that fail to
> extract should now be the fault of the package. Hopefully. We'll see if the
> full archive has an interesting edge cases.

I'm running a full oneiric extraction now, it will be interessting to
see what it outputs and I will compare to the previous runs to find
regressions (but I really doubt there will be any).

Thanks,
 Michael