On Thu, Sep 16, 2010 at 10:22:13AM +0200, Tshepang Lekhonkhobe wrote:
> Package: python-apt
> Version: 0.7.97.1
> Severity: normal
>
> Please find attached.
>
>
> --
> blog: http://tshepang.tumblr.com
> === modified file 'apt/cache.py'
> --- apt/cache.py 2010-09-03 10:08:20 +0000
> +++ apt/cache.py 2010-09-15 22:09:43 +0000
> @@ -46,15 +46,15 @@
> class Cache(object):
> """Dictionary-like package cache.
>
> - This class has all the packages that are available in it's
> - dictionary.
> + This class has all available packages in its dictionary.
Both are correct semantically, although none is better.
>
> Keyword arguments:
> - progress -- a OpProgress object
> - rootdir -- a alternative root directory. if that is given
> - the system sources.list and system lists/ files are
> - not read, only files relative to the given rootdir
> - memonly -- build the cache in memory only
> +
> + * progress -- an OpProgress object
> + * rootdir -- an alternative root directory; the system sources.list
> + and lists/files are skipped in favour of files relative to the
> + rootdir if given
> + * memonly -- build the cache in memory
> """
>
> def __init__(self, progress=None, rootdir=None, memonly=False):
> @@ -115,7 +115,7 @@
> callback()
>
> def open(self, progress=None):
> - """ Open the package cache, after that it can be used like
> + """ Open the package cache, after which it can be used like
> a dictionary
> """
> if progress is None:
Should probably be rewritten completely
> @@ -173,8 +173,9 @@
> yield self[pkgname]
> raise StopIteration
>
> - def has_key(self, key):
> - return (key in self._set)
> + def has_key(self, pkgname):
> + "Return ``True`` if pkg `pkgname` is found in the cache."
> + return (pkgname in self._set)
Unused, completely deprecated due to Python 3. Everyone knows that
d.has_key(k) == k in d anyway.
> def __contains__(self, key):
> return (key in self._set)
> @@ -183,10 +184,11 @@
> return len(self._set)
>
> def keys(self):
> + "Return a list of all package names in the cache."
> return list(self._set)
Makes sense, but always use 3 quotes.
> def get_changes(self):
> - """ Get the marked changes """
> + "Get the marked changes."
> changes = []
> marked_keep = self._depcache.marked_keep
> for pkg in self._cache.packages:
> @@ -202,9 +204,9 @@
> def upgrade(self, dist_upgrade=False):
> """Upgrade all packages.
>
> - If the parameter *dist_upgrade* is True, new dependencies will be
> + If the parameter *dist_upgrade* is ``True``, new dependencies will be
> installed as well (and conflicting packages may be removed). The
> - default value is False.
> + default value is ``False``.
> """
> self.cache_pre_change()
> self._depcache.upgrade(dist_upgrade)
I did not want rST in the docstrings, because I saw them as a short reference,
with a longer and more detailed one included in the one created by Sphinx.
> @@ -220,7 +222,7 @@
>
> @property
> def required_space(self):
> - """Get the size of the additional required space on the fs."""
> + """Get the size of the additional required space on the filessystem."""
> return self._depcache.usr_size
Should be "file system", not "filessystem".
> """
> @@ -328,13 +330,14 @@
> @deprecated_args
> def update(self, fetch_progress=None, pulse_interval=0,
> raise_on_error=True, sources_list=None):
> - """Run the equivalent of apt-get update.
> -
> - The first parameter *fetch_progress* may be set to an instance of
> - apt.progress.FetchProgress, the default is apt.progress.FetchProgress()
> - .
> + """Run the equivalent of ``apt-get update``.
> +
> + The first parameter, *fetch_progress*, may be set to an instance of
> + :class:`apt.progress.FetchProgress` (default is
> + :class:`apt.progress.base.AcquireProgress()`)
Seems a bit incomplete from a grammatical point of view.
> +
> sources_list -- Update a alternative sources.list than the default.
> - Note that the sources.list.d directory is ignored in this case
> + Note that the sources.list.d directory is ignored in this case
> """
As that's basically a list, the extra indentation helps to get context. I
prefer a normal text style, though, as this is for humans, not machines (see
prior rST stuff above)
>
> def cache_post_change(self):
> - " called internally if the cache has changed, emit a signal then "
> + "Called internally if the cache has changed, emit a signal then."
> self._run_callbacks("cache_post_change")
>
> def cache_pre_change(self):
> - """ called internally if the cache is about to change, emit
> - a signal then """
> + """Called internally if the cache is about to change, emit
> + a signal then."""
> self._run_callbacks("cache_pre_change")
Those two should probably be rewritten to fit into the
"Summary.
Long description"
style.
> @@ -461,20 +464,20 @@
> self._callbacks[name].append(callback)
>
> def actiongroup(self):
> - """Return an ActionGroup() object for the current cache.
> + """Return an :class:`ActionGroup()` object for the current cache.
>
> - Action groups can be used to speedup actions. The action group is
> + Action groups can be used to speed-up actions. The action group is
> active as soon as it is created, and disabled when the object is
> deleted or when release() is called.
According to the American Heritage® Dictionary of the English Language, speedup
is the correct form.
>
> - You can use the action group as a context manager, this is the
> + You can use the action group as a context manager. This is the
> recommended way::
I prefer a semicolon here.
>
> with cache.actiongroup():
> for package in my_selected_packages:
> package.mark_install()
>
> - This way, the ActionGroup is automatically released as soon as the
> + This way, the action group is automatically released as soon as the
> with statement block is left. It also has the benefit of making it
> clear which parts of the code run with a action group and which
> don't.
OK.
> === modified file 'apt/cdrom.py'
> --- apt/cdrom.py 2009-07-31 13:24:09 +0000
> +++ apt/cdrom.py 2010-09-15 22:18:00 +0000
> @@ -28,20 +28,16 @@
>
>
> class Cdrom(apt_pkg.Cdrom):
> - """Support for apt-cdrom like features.
> -
> - This class has several optional parameters for initialisation, which may
> - be used to influence the behaviour of the object:
> -
> - The optional parameter `progress` is a CdromProgress() subclass, which will
> - ask for the correct cdrom, etc. If not specified or None, a CdromProgress()
> - object will be used.
> -
> - The optional parameter `mountpoint` may be used to specify an alternative
> - mountpoint.
> -
> - If the optional parameter `nomount` is True, the cdroms will not be
> - mounted. This is the default behaviour.
> + """Support for ``apt-cdrom``-like features.
> +
> + All parameters for this class are optional:
> +
> + * `progress` is a CdromProgress() subclass, which will ask for the
> + correct cdrom, etc. If not specified or None, a :class:`CdromProgress()`
> + object will be used.
> + * `mountpoint` may be used to specify an alternative mount point.
> + * If the `nomount` is True, the cdroms will not be mounted. This is
> + the default behaviour.
> """
I prefer the previous text form.
>
> def __init__(self, progress=None, mountpoint=None, nomount=True):
>
> === modified file 'apt/debfile.py'
> --- apt/debfile.py 2010-09-07 12:02:50 +0000
> +++ apt/debfile.py 2010-09-15 22:27:23 +0000
> @@ -16,7 +16,7 @@
> # along with this program; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> # USA
> -"""Classes for working with locally available Debian packages."""
> +"""Classes for working with locally-available Debian package files."""
No, package files are actually the 'Packages' files in APT terminology,
see apt_pkg.PackageFile. I once thought about 'package archive', but
we have a meaning for archive as well (the distribution's name; e.g.,
unstable).
> @@ -31,7 +31,7 @@
> """Exception which is raised if a file is no Debian archive."""
>
> class DebPackage(object):
> - """A Debian Package (.deb file)."""
> + """An object representing a Debian Package file (.deb)."""
Same here.
> @@ -299,10 +299,10 @@
>
> def check_breaks_existing_packages(self):
> """
> - check if installing the package would break exsisting
> - package on the system, e.g. system has:
> + Check if installing the package would break exsisting
> + package(s) on the system, e.g. system has:
You should fix the typo as well, then.
> smc depends on smc-data (= 1.4)
> - and user tries to installs smc-data 1.6
> + and user tries to installs smc-data 1.6.
installs should be install, then.
>
> === modified file 'apt/package.py'
> --- apt/package.py 2010-08-27 13:06:54 +0000
> +++ apt/package.py 2010-09-15 20:48:55 +0000
> @@ -208,6 +208,16 @@
>
> class Version(object):
> """Representation of a package version.
> +
> + One way to create an instance::
> +
> + cache = apt.Cache()
> + pkg = cache["python-apt"]
> + pkgv = pkg.candidate
> +
> + And another not-so-readable way::
> +
> + pkgv = apt.Cache()["python-apt"].candidate
>
> .. versionadded:: 0.7.9
> """
See the rST discussion; and the second form can be
derived from the first one, and should never be used.
> @@ -300,7 +310,7 @@
>
> @property
> def downloadable(self):
> - """Return whether the version of the package is downloadable."""
> + """Return ``True`` if the version of the package is downloadable."""
Maybe.
All in all, please separate your patches:
(1) Spelling/Grammar fixes
(2) rST changes
(3) additions
I will accept (1) definitively, the others need some more thoughts.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
Attachment:
pgpOcxmASDeFV.pgp
Description: PGP signature