Hi Kevin, Some comments on the first patch, and a rejection of the second patch. See below.
(Also, you made me realise that I have bungled yet another function.
Thanks! See below)
On Fri, Nov 16, 2007 at 12:39:40 -0700, [EMAIL PROTECTED] 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"
> +maybeGetEnv :: String -> IO (Maybe String)
> +maybeGetEnv s = (getEnv s >>= return.Just) `catchall` return Nothing -- err
> can only be isDoesNotExist
Likewise (Just `fmap` getEnv s)?
> +-- |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?
> +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.
-----------------------------------------------------------------------
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.
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!
===============================================
Note: getCurrentDirectorySansDarcs is messed up
===============================================
Staring at your patch makes me realise how terribly messed up
getCurrentDirectorySansDarcs is. (erician mortification). We should fix
this somewhat urgently, as it may hurt people that neither have a /tmp
directory nor a $DARCS_TEMP.
The overall intention of getCurrentDirectorySansDarcs is to prevent
temporary directories from being created in the pristine cache. To
achieve this, we truncate the path before "_darcs". For example,
helloworld/_darcs/ ==> helloworld/
helloworld/_darcs/bar/baz ==> helloworld/
Here's a pictoral representation of how it works
h
he
....
helloworld <== selected via head.reverse (basically)
---------------------------------------------------
the next block of entries are eliminated via drop 5
(_darcs has 6 chars)
---------------------------------------------------
helloworld/_
helloworld/_d
helloworld/_da
helloworld/_dar
helloworld/_darc
------------------------------------------
the below is cut off by takeWhile no_darcs
------------------------------------------
helloworld/_darcs
...
helloworld/_darcs/bar/baz
Now here are the bugs in my code!
(Bug #1) isPrefixOf instead of isSuffixOf. My fix for a silly
inefficiency was worse than silly; it was wrong! The consequence
of this wrongness is that getCurrentDirectorySansDarcs systematically
chops off 5 characters from the current path
(Bug #2) it doesn't know the difference between _darcs and /_darcs
i_like_darcs ==> i_like
(Bug #3) it doesn't deal with paths that do not contain a _darcs
component
helloworld ==> hello
There is a simple simple function lurking around in this complicated
mess. Given a path, I just want to see to it that it gets chopped
off from /_darcs (inclusive), if it is necessary to do so.
--
Eric Kow http://www.loria.fr/~kow
PGP Key ID: 08AC04F9 Merci de corriger mon français.
pgpWpmXr7gupU.pgp
Description: PGP signature
_______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
