[darcs-users] [patch338] Adding some haddock to Cache.hs

Eric Kow kowey at darcs.net
Tue Aug 10 16:59:24 UTC 2010


Adding some haddock to Cache.hs
-------------------------------
I'm always very grateful for haddocks!  One day we'll have a proper
Darcs API, stable, clean, sensible and appropriately documented, and
you have just contributed to the cause.

That said, I have suggestions...

> +-- | @fetchFileUsingCache@ receives the hash of the file, the repository
> +-- in which is going to be located, and the directory for which the
> +-- hashed file belongs.

I'm a bit confused by this, because the type signature seems to be
pointing at less stuff.

Also perhaps you could make this a little bit more terse by using the
annotation on type signature style?

fetchFileUsingCache :: Cache
                    -> HashedDir -- ^ target repo
                    -> String    -- ^ hash
                    -> IO (String, B.ByteString)

(I'm not sure what the best practice is, just exploring, eh?)

>     It uses @fetchFileUsingCachePrivate@ to fetch
> +-- the file

No, that's an implementation detail that I don't care about if
I'm using the Darcs API.

> , it takes as source the first entry of the cache, it the
> +-- file is not in that source, it tries to get for the next one. It
> +-- will transverse the cache list until it is empty or it fetches the
> +-- file, if the file can't be fetch the operation fails.

Ah, that's a tricky one to word because you want to convey three
ideas: trying the sources in order, returning on first success
and failing if you exhaust the sources.

Hmm, how about something like this?

  It tries to fetch the file from one of the sources, trying
  them in order one by one.  If the file cannot be fetched
  from any of the sources, this operation fails.

Phew! This is hard! I hope you don't mind my constant rewording.
Submitting the haddock patch is a very important first step because
it calls our attention to what needs expressing.

> +-- | copyFileUsingCache, copy or hardlinks a file to the cache, usually the global cache, if
> +--   the cache directory doesn't exist the function will fail.
>  copyFileUsingCache :: OrOnlySpeculate -> Cache -> HashedDir -> String -> IO ()
>  copyFileUsingCache oos (Ca cache) subdir f =
>      do debugMessage $ "I'm doing copyFileUsingCache on "++(hashedDir subdir)++"/"++f

First, I think using a convention like

@copyFileUsingCache oos ca dir f@ copies the file @f@ to the cache @ca@

makes the haddock a bit clearer because it shows you which arguments
refer to which high-level concept (this matters most when you have 
arguments of the same type, which is not the case here)

Second, you should split into two sentences, before "if the cache
directory".

Thanks!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100810/6720f25c/attachment.pgp>


More information about the darcs-users mailing list