[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Request for review: python-apt inline documentation



On Wed, Apr 14, 2010 at 02:33:38AM -0500, Jonathan Nieder wrote:
 
> > DESCRIPTION
> >     The apt_pkg module provides several classes and functions for accessing
> >     the functionality provided by the apt-pkg library.
> 
> Is the apt-pkg library at /usr/lib/libapt-pkg-<something>, as provided
> by the apt package?  Where can I find out more about it?
Well, apt-pkg is severely under-documented (in my opinion). And I don't
think it's relevant here.


> >     class Acquire(__builtin__.object)
> >      |  Acquire(progress: apt_pkg.AcquireProgress) -> Acquire()
> >      |  
> >      |  Create a new Acquire object.
> 
> Yes, but what is it for?
> 
>   An Acquire object is used to retrieve files from a remote
>   repository.
Well, technically it just coordinates them; and as you might
know, APT also supports copy:/ urls for local files, so I
probably take something like:

    Coordinate the retrieval of files via network or local file system
    (using 'copy:/path/to/file' style URIs). 
> 
> >      |  The parameter 'progress' can be used to
> >      |  specify an apt_pkg.AcquireProgress() object, which will display the
> >      |  progress of the fetching.
> 
> “Progress” and “fetching” are generally considered amorphous
> substances that don’t deserve an article.  Maybe one of the
> two phrases below would be clearer:
> 
>   which will display progress information during the fetch.
>   which will be used to report progress to the caller.
I guess it does not have to be passive and could be:

	which will report progress information.

and change will to 'may' or 'might', since it may be just
a dummy implementation.


> 
> >      |  run(...)
> >      |      run() -> int
> >      |      
> >      |      Run the fetcher and return one of RESULT_CANCELLED,
> >      |      RESULT_CONTINUE, RESULT_FAILED.
> 
>   Start fetching.  Returns RESULT_CANCELLED, RESULT_CONTINUE, or
>   RESULT_FAILED, depending on how well that went.
It does not just start fetching, but also waits until it
finished (well, one could call run() first and then add
items as well, but if there are already items in the queue,
run() will return after they have been fetched).



> 
> What do the result codes mean?  Does RESULT_CONTINUE represent
> success or does it mean the operation didn’t finish yet?  What
> happens if there is nothing to fetch?

I added:
     "[...]. RESULT_CONTINUE means that all items\n"
     "which where queued prior to calling run() have been fetched\n"
     "successfully. RESULT_CANCELLED means that the process was canceled\n"
     "by the progress class. And RESULT_FAILED means a generic failure."


> 
> >      |  shutdown(...)
> >      |      shutdown()
> >      |      
> >      |      Shut the fetcher down, removing all items from it.
> 
> What does it mean to remove all items from a fetcher?  If I
> restart the same fetch later, will it be able to use the partial
> result?
It's now:
     "Shut the fetcher down, removing all items from it. Future access to\n"
     "queued AcquireItem objects will cause a segfault. The partial result\n"
     "is kept on the disk and not removed and APT might reuse it."

> 
> >      |  fetch_needed
> >      |      The total amount of data to be fetched.
> 
> Units?  (Or does the type indicate that already?)
I added "(number of bytes)" to all of them.


> 
> >      |  items
> >      |      A list of all items as apt_pkg.AcquireItem objects.
> 
> Does this include items already fetched?
     "A list of all items as apt_pkg.AcquireItem objects, including already\n"
     "fetched ones and to be fetched ones."

> 
> [...]
> >      |  workers
> >      |      A list of all active workers as apt_pkg.AcquireWorker objects.
> 
> Do these correspond to threads?  How would a caller interact with
> them?
They are just normal objects and intended to be used in a normal
fashion by accessing attributes, etc. And how they are used is described
in the AcquireWorker class.

> 
> [...]
> >     class AcquireFile(AcquireItem)
> >      |  AcquireFile(owner, uri[, md5, size, descr, short_descr, destdir,destfile])
> >      |  
> >      |  Represent a file to be fetched.
> 
> For Acquire, you described what the constructor does; here, you
> are describing what the object does.  It would be nice to follow
> whatever is standard practice in wording these things for Python
> classes; unfortunately, I don’t know what the usual convention
> is.
The Acquire part was older and not part of my recent changes and
thus I seem to have overlooked it.

> 
> [...]
> >      | The destination can be specified by either 'destdir'
> >      |  for specifying the destination directory or 'destfile' for
> >      |  specifying the path to the destination file.
> 
>   The destination can be specified as a directory using the 'destdir'
>   parameter or as a path to a file using 'destfile'.  At most one
>   of these parameters should be not equal to None.  If no
>   destination is specified, then...
Changing it to:

    Normally, the file will be stored in the current directory using the
    file name given in the URI. This directory can be changed by passing
    the name of a directory to the *destdir* parameter. It is also possible
    to set a path to a file using the *destfile* parameter, but both can
    not be specified together.

> 
> >      |  
> >      |  The parameters 'short_descr' and 'descr' can be used to specify
> >      |  a short description and a longer description for the item. This
> >      |  information is used by progress classes to refer to the item.
> 
> How long should these descriptions be?  (e.g., a few words, one line,
> one paragraph)
    "The parameters 'short_descr' and 'descr' can be used to specify\n"
    "a short description and a longer description for the item. This\n"
    "information is used by progress classes to refer to the item and\n"
    "should be short, for example, package name as *short_descr* and\n"
    "and something like 'http://localhost sid/main python-apt 0.7.94.2'\n"
    "as *descr*."

> 
> [...]
> >      |  id
> >      |      The ID of the item. An integer which can be set by progress classes.
> 
> From the point of view of AcquireFile(), is this just a place
> for callers to store some data, or is it used as a parameter to
> some callbacks?
It's part of the progress classes definition. TextProgress,
for example, sets it in an order and displays it so you know
which item was started when (it is the number following 'Get:'
in apt-get's output).



> 
> >      |  is_trusted
> >      |      Whether the item is trusted or not.
> 
> Trusted by whom?  I guess this indicates whether it was signed using a
> signature in the APT keyring.  Or "trusted according to the APT
> configuration".
Becomes:
     "Whether the item is trusted or not. Only True for packages\n"
     "which come from a repository signed with one of the keys in\n"
     "APT's keyring."



> 
> >      |  status
> >      |      The status of the item.
> 
> How does it compare to mode?  (At first, I guessed this had something
> to do with the status of a package.)

From the long description:
        Integer, representing the status of the item. This attribute can be
        compared against the following constants to gain useful information
        on the item's status.
The short one would be:
	An integer representing the item's status which can be compared
	against one of the STAT_* constants defined in this class.

> 
> >      |  STAT_AUTH_ERROR = 4
> 
>   There was an error in the credentials used to fetch the file.

Yes, but we can't document constants. And they are fairly
self-explanatory anyway. The longer documentation contains
explanations for them, but for the short one, their sole
existence is explanation enough.

>   Abstract class representing a item to be fetched by an Acquire
>   object.
Just "Represent a single item to be fetched by an Acquire object.\n\n"
now. The paragraph about not being able to create subclasses is
sufficient to see that this class is abstract.

> 
> Why does this class exist separately from AcquireItem?  Is it possible
> to make my own subclass with different behavior?
There are many subclasses in apt-pkg which are just not exported
in python-apt. I added
	"It is not possible to create subclasses on the
	Python level, only on the C++ level."



> 
> >     class AcquireWorker(__builtin__.object)
> >      |  Acquire workers represent exactly one subprocess used for fetching
> >      |  files.
> 
> Another convention. :)
> 
> Presumably each AcquireWorker object represents one process, rather
> than all of them taken together representing one.
Of course (see next point).

> 
> >      | This subprocess is created from the methods stored in
> >      |  Dir::Bin::Methods.
> 
> language tweak: s/from the methods stored in/using methods from/

    "Represent a sub-process responsible for fetching files from\n"
    "remote locations. This subprocess uses 'methods' located in\n"
    "the directory specified by the configuration option\n"
    "Dir::Bin::Methods.";

> 
> >      |  resumepoint
> >      |      The amount of data which was already fetched prior to resuming the
> >      |      download.
> 
>   The amount of data which was already fetched when the download was
>   last resumed.
Changed to:
    "The amount of data which was already available when the download was
     started."
This better explains it and avoids some confusion, since APT does
not pause and resume downloads, but just resumes a download when you
start it and there was already something available on the disk.

> >      |  status
> >      |      The status of the worker, as a string.
> 
> This string would be a few words long, I assume.
Probably the same as AcquireItem.mode, but I haven't
checked it. It's one word. (It's the line in apt-get
output which contains many '[string] [string]' parts
whereas 'string' is the value of this property.


All in all, this was helpful. I also found some other
mistakes while fixing the ones you found.


-- 
Julian Andres Klode  - Debian Developer, Ubuntu Member

See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.

Attachment: pgpye9C8sEluJ.pgp
Description: PGP signature


Reply to: