Merge lp:~bjornt/landscape-client/apt-get-packages-versions into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Change AptFacade.get_packages() to return one object per version of a
package, so that we can report all the installed and available package
versions.

[Jerry Seutter (jseutter)]

+1, looks good.

[1]
 + def is_package_available(self, version):
 """Is the package available for installation?"""

is_package_available is only checking if the specific version of the package is available. The docstring/method name could be a bit more precise. This also may be nitpicking :)

I guess this raises a bigger question: Is a version a package? I'm curious because get_package_by_name returns a version...

review: Approve

[lp:~bjornt/landscape-client/apt-get-packages-versions]

435. By Björn Tillenius on 2011-10-12

Merge trunk.

[Free Ekanayaka (free.ekanayaka)]

Looks good, +1!

[1]

     def get_packages(self):
         """Get all the packages available in the channels."""
- return self._pkg2hash.keys()
+ return self._hash2pkg.values()

I believe get_packages is mostly used to iterate over the available packages, maybe we can replace or add an iter_packages() method returning self._hash2pkg.itervalues() and saving some memory/CPU.

[2]

+ return [
+ version for version in self.get_packages()
+ if version.package.name == name]

Maybe this method is only used in tests, but if not we might want to create an index as well to improve performance.

review: Approve

[Björn Tillenius (bjornt)]

> Looks good, +1!
>
> [1]
>
> def get_packages(self):
> """Get all the packages available in the channels."""
> - return self._pkg2hash.keys()
> + return self._hash2pkg.values()
>
> I believe get_packages is mostly used to iterate over the available packages,
> maybe we can replace or add an iter_packages() method returning
> self._hash2pkg.itervalues() and saving some memory/CPU.

Sure, makes sense. I think it would be better to simply return itervalues() from there, though, no need to add another method.
>
> [2]
>
> + return [
> + version for version in self.get_packages()
> + if version.package.name == name]
>
> Maybe this method is only used in tests, but if not we might want to create an
> index as well to improve performance.

I'll go through the callsites to see if it makes sense.

[Björn Tillenius (bjornt)]

> +1, looks good.
>
>
> [1]
> + def is_package_available(self, version):
> """Is the package available for installation?"""
>
> is_package_available is only checking if the specific version of the package
> is available. The docstring/method name could be a bit more precise. This
> also may be nitpicking :)
>
> I guess this raises a bigger question: Is a version a package? I'm curious
> because get_package_by_name returns a version...

So, this is a bit of a mismatch between smart an apt. In smart a package is a specific version of a package, where in apt, a package is a container that has a list of available versions. It would probably be good to try to clean it up, but it would be better to do that after dropping smart, since renaming methods before that would make the migration work harder.

[lp:~bjornt/landscape-client/apt-get-packages-versions]

436. By Björn Tillenius on 2011-10-12

Return an iterator from get_packages() to save memory.