[prev in list] [next in list] [prev in thread] [next in thread] 

List:       darcs-devel
Subject:    Re: [darcs-devel] darcs patch: Added maybeGetEnv and firstJustIO
From:       Kevin Quick <quick () sparq ! org>
Date:       2007-11-19 5:19:22
Message-ID: 0CA2CBB4-BE01-415D-98E3-481E1668C485 () sparq ! org
[Download RAW message or body]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

- -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On 18 Nov 2007, at 8:48 PM, Eric Y. Kow wrote:

> Hi Kevin,
>
> Some comments on the first patch, and a rejection of the second patch.
> See below.
>
> On Fri, Nov 16, 2007 at 12:39:40 -0700, quick@sparq.org wrote:
>> +get_easy_author = firstJustIO [ get_preflist "author" >>=  
>> return . firstNotBlank,
>> +                                get_global "author" >>=  
>> return .firstNotBlank,
>
> Would this be an improvement?
>   firstNotBlank `fmap` get_global "author"

I'd think it is purely personal preference.  My form is easier for  
*me* to understand,
since it follows basic monad operational principles and because I  
haven't fully grokked
the 'fmap' operations yet.  I'm assuming it's the same functionality  
or else you wouldn't
have proposed it, so I have no objection to that (and perhaps it fits  
better with David's
"point-free" hesitance).

>
>> +maybeGetEnv :: String -> IO (Maybe String)
>> +maybeGetEnv s = (getEnv s >>= return.Just) `catchall` return  
>> Nothing -- err can only be isDoesNotExist
>
> Likewise (Just `fmap` getEnv s)?

Again, personal preference, and the former is easier for me to read.   
It's also similar in form to "getEnvBool" in ColourPrinter.lhs  
(although to truly follow form, mine should be "getEnvMaybe").

>
>> +-- |The firstJustM returns the first Just entry in a list of  
>> monadic operations.  This is close to
>> +--  `listToMaybe `fmap` sequence`, but the sequence operator  
>> evaluates all monadic members of the
>> +--  list before passing it along (i.e. sequence is strict).
>
> Did you actually verify this, or is it just what the documentation
> would lead one to believe?

I verified this using trace.  I don't have the actual test source in  
front of me at the moment,
but it was something like:

import Data.Trace
listToMaybe `fmap` sequence [ return (Just "start"),
                                                          maybeGetEnv  
(trace "A" "first"),
                                                          maybeGetEnv  
(trace "B" "second"),
                                                          maybeGetEnv  
(trace "C" "third") ]
Which output:
A
B
C
Just "start"

 From which I inferred that the IO monad for every element in the  
list had been executed, when just the first should have been  
sufficient for lazy evaluation.

>
>> +firstJustM :: Monad m => [m (Maybe a)] -> m (Maybe a)
>> +firstJustM [] = return Nothing
>> +firstJustM (e:es) = e >>= (\v -> if isJust v then return v else  
>> firstJustM es)
>
> Hmm, something like >>= fromMaybe (firstJustM es) might work as well,
> or maybe it's just making things incomprehensible.

That seems to stumble over types (specifically the firstJustM result  
is encapsulated in the IO
monad, which doesn't fit with how fromMaybe wants to work with its  
arguments).

More generally, I wanted to ensure the optimization wherein  
subsequent elements of the list were not executed once one had return  
a Just value... and I'm not sure if fromMaybe is lazy enough about  
evaluating its first argument to provide that optimization.

>
> ---------------------------------------------------------------------- 
> -
> Simplify tempdir_loc with firstJustIO; fix  
> getCurrentDirectorySansDarcs
> ---------------------------------------------------------------------- 
> -
>
> In principle, I do like the overall simplification.  But there are
> a couple of things that make me want to hold off a bit.
>
> First objection:
>> return $ Just "."  -- always returns a Just
>
> I am so sure we should do that.
>  1. I don't know if using "./" is always a good last resort for a
>     temporary directory.
>  2. getCurrentDirectorySansDarcs already does the same thing, more
>     or less, so by rights we should never fall through to this case.
>     places we should never fall through to should be given good old
>     'impossible' so that they will blow up instead of going silent
>     but deadly on us
>
> Second objection: I feel somewhat uncomfortable about the idea of  
> using
> fromJust and then forcing a Just so that the fromJust would always  
> work.
> That seems rather fragile, and makes me wonder if there isn't a  
> simpler
> way out.

The first and second objections are tied together in my mind, so I'll  
answer them both at the same time.

Basically, I viewed it as "get a temporary directory from this list  
of possible locations in order of preference".  If all other location  
attempts failed, I had thought it would be better to just use the  
current directory rather than throw a fatal exception that was likely  
to annoy the user.  This assumes that the current directory is "safe"  
however... debatable if it's within the pristine, although it *is* a  
temporary directory, so ...

    * The first entry fails if there's no _darcs/prefs entry
    * The next couple of tries will fail if environment variables  
aren't set
    * The fourth will fail if it's not a Un*x style filesystem that  
has "/tmp"
    * The fifth form, tries to move to the top of the darcs working  
directory (i.e. the directory
       that contains "_darcs").
    * The sixth provided a constant default (the current directory)

It's easy to see that the first four could fail.  As discussed below,  
I think even the fixed version of the fifth  
(getCurrentDirectorySansDarcs) could also fail, ergo always using the  
current directory seemed like a better user experience than throwing  
impossible given that the previous 5 could fail.

With my adjustment to getCurrentDirectorySansDarcs, it also would  
return Nothing if it failed, rather than throwing an impossible  
exception.  For example, getCurrentDirectorySansDarcs could fail if  
the darcs command was run from outside (above) the repo directory  
entirely and the --repodir was used.  In that case, I believe that  
even the fixed version of the old form could throw impossible.

Thus the (badly commented) final entry in the array was meant to  
inform that it was the final default that would be taken if none of  
the preceding succeeded.  And keeping that "final default" in the  
array of entries to check made more sense to me than having it  
internal to getCurrentDirectorySansDarcs.

>
> Third objection:
>> +getCurrentDirectorySansDarcs :: IO (Maybe FilePath)
>> hunk ./src/Darcs/Lock.lhs 185
>> - -  case drop 5 $ reverse $ takeWhile no_darcs $ inits c of
>> - -    []    -> impossible
>> - -    (d:_) -> return d
>
> Here, we really wanted 'impossible'.  This is a bit similar to
> the first case.  We want it to blow up!

Per my analysis above, I'm not entirely sure I agree.  However, if  
the current directory is too dangerous of a default, I'd prefer a:  
bug "Cannot determine location for temporary directory; try setting  
DARCS_TMPDIR" rather than just "impossible"  (as an entry in the  
tempdir_loc array rather than in getCurrentDirectorySansDarcs).

> -- 
> Eric Kow                     http://www.loria.fr/~kow
> PGP Key ID: 08AC04F9         Merci de corriger mon français.

Va bien,
   -KQ

- -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHQRzOt76lKrRL0ewRAkwFAJ911wh0x8xOoDROOLyQp4MDhgbBPACghXSj
3CnNQ4ne/Bj96hWc6ftjVBE=
=T4cD
- -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHQRzat76lKrRL0ewRAqg+AJsHhuet80wSAx560fITsobY+HC6XgCfcP30
s/MD7phjJjn7FMrHxxglmQQ=
=9uol
-----END PGP SIGNATURE-----
_______________________________________________
darcs-devel mailing list
darcs-devel@darcs.net
http://lists.osuosl.org/mailman/listinfo/darcs-devel

[prev in list] [next in list] [prev in thread] [next in thread]