On Fri, Aug 21, 2015 at 11:43:41AM +0200, Michael Schaller wrote:
> On 08/20/2015 10:07 PM, Julian Andres Klode wrote:
> >On Thu, Aug 20, 2015 at 09:58:11PM +0200, Michael Schaller wrote:
> >
> >Hi Michael, I think it looks OK, but let me suggest some
> >changes:
> >
> >- Drop the parent_version property and use _version directly
> >- Drop the cache property and use _cache directly
> >- Drop the TargetVersionList. AFAIK, the only thing different
> > is it wrapping the list representation in a <TargetVersionList>
> > thing, this does not seem to be needed (there's no point
> > creating our own list type just for some corner-case
> > cosmetic issue).
> >
> >Then the patch will be much smaller, and still be able to do
> >the same.
> >
>
> I can remove the 'parent_version' and 'cache' properties but would like to
> know your reasoning. From my point of view these properties just make the
> code more consistent with the other properties that are already there. It
> is unfortunate though that I can't name the 'parent_version' property
> 'version' as that would be even more consistent.
There are no parent references in the other objects, and it is
completely unrelated/unneeded to the patch in question anyway.
>
> I would also like to keep the 'TargetVersionList' class as I quite often use
> the interactive Python console with python-apt during development and there
> it helps a lot to have a good '__repr__' implementation.
>
> Take for an example this interactive Python session. I've replaced '>>> '
> with a newline for better readability/mailability.
>
> $ python
> import apt
>
> cache = apt.Cache()
>
> cache
> <apt.cache.Cache object at 0x7fef993d6210>
>
> pkg = cache['bash']
>
> pkg
> <Package: name:'bash' architecture='amd64' id:738L>
>
> pkg.cache
> <apt.cache.Cache object at 0x7fef993d6210>
>
> ver = pkg.installed
>
> ver
> <Version: package:'bash' version:'4.3-11ubuntu2'>
>
> ver.package
> <Package: name:'bash' architecture='amd64' id:738L>
>
> deps = ver.get_dependencies('Depends')
>
> deps
> [<Dependency: [<BaseDependency: name:'base-files' relation:'>='
> version:'2.1.12' rawtype:'Depends'>]>, <Dependency: [<BaseDependency:
> name:'debianutils' relation:'>=' version:'2.15' rawtype:'Depends'>]>]
>
> deps[0]
> <Dependency: [<BaseDependency: name:'base-files' relation:'>='
> version:'2.1.12' rawtype:'Depends'>]>
>
> deps[0].parent_version
> <Version: package:'bash' version:'4.3-11ubuntu2'>
>
> deps[0].target_versions
> <TargetVersionList: "[<Version: package:'base-files' version:'7.2ubuntu9'>,
> <Version: package:'base-files:i386' version:'7.2ubuntu9'>]">
The only advantage is the prefix, and that's not enough. This is a
dynamically typed language after all, there's no point having a
special type, that's completely non-Pythonic.
We only have this for versions in VersionList, because we need/want
to treat it like a dictionary.
>
>
> BTW: From my point of view 'Version.__repr__' should also include the
> architecture and the 'id' in 'Package.__repr__' is rather useless.
The architecture *is* included - implicitly, in the name - it is the
pretty-printed package name, that is the name for the native arch,
and name:arch for a foreign arch.
The ID is a bit useless, but there's no point dropping it and it
can help in some situations where you accidentally have objects
from different caches.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
Be friendly, do not top-post, and follow RFC 1855 "Netiquette".
- If you don't I might ignore you.
