Code review comment for lp:~bjornt/landscape-client/apt-facade-install-package

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Nice, I just have a few questions. +1

[1]

+ version.package.candidate = version

Can you elaborate a bit on why this is needed and what does it do? A comment would be good.

And these two lines too:

+ fixer.clear(version.package._pkg)
+ fixer.protect(version.package._pkg)

[2]

+ versions_to_be_installed = set(
+ package.candidate for package in self._cache.get_changes())
+ dependencies = versions_to_be_installed.difference(
+ self._package_installs)

I'm not sure what the get_changes() API returns, does this handle packages to be upgraded and removed as well?

[3]

+ extra_items=None):

Maybe s/extra_items/control_fields/ would be more clear.

[4]

+ def commit():
+ self.committed = True

Instead of creating an attribute on the test case it'd be more isolated to append an item to a local list, like:

        committed = []
        def commit():
            committed.append(True)

review: Approve

« Back to merge proposal