[darcs-devel] [patch527] resolve issue2003: no set-default hint i... (and 1 more)
Eric Kow
kowey at darcs.net
Fri Jan 21 17:39:05 UTC 2011
Should have pricked my ears when yo said you wanted to get that
branch out the door for HP.
Generally +1 idea, +1 implementation with minor comments about
the UI stuff and some implementation niggles.
I'm going to see if I can push the first and third patch to
screened/unstable. Second patch at your discretion.
On Fri, Jan 07, 2011 at 21:58:00 +0000, Ganesh Sittampalam wrote:
> Fri Jan 7 20:36:17 GMT 2011 Ganesh Sittampalam <ganesh at earth.li>
> * resolve issue2003: no set-default hint if user explicitly chose no-set-default
> Fri Jan 7 20:36:19 GMT 2011 Ganesh Sittampalam <ganesh at earth.li>
> * add instruction on suppressing set-default message
>
> Fri Jan 7 20:36:30 GMT 2011 Ganesh Sittampalam <ganesh at earth.li>
> * add test of set-default hint messages
resolve issue2003: no set-default hint if user explicitly chose no-set-default
------------------------------------------------------------------------------
I couldn't figure out how to do this because I wasn't thinking out of
the pre-existing flag box. :-)
My only real comment is that I'd probably have an easier time reading
the code if we replaced the Bool with something self-explanatory like
ExplicitlySet.
This is a nice hack for expediency. In the long term, we need to
overhaul this flags stuff, hopefully extending the notion of implicitly
and explicitly set options accordingly. Sort of a useful thing for Neil
and Petr (cmdargs and cmdlib) to know about.
> +(DarcsInternalOption f) `isin` fs = f `elem` fs
I didn't check if there are any other pattern matches
> + | DarcsInternalOption DarcsFlag
> + -- ^ @DarcsInternalOption@
> + -- An option just for internal use (e.g. defaulting), not directly available to the user.
We introduce this because we don't want to display the help for
> -optionFromDarcsAtomicOption :: AbsolutePath -> DarcsAtomicOption -> OptDescr DarcsFlag
> +optionFromDarcsAtomicOption :: AbsolutePath -> DarcsAtomicOption -> Maybe (OptDescr DarcsFlag)
> +optionFromDarcsAtomicOption wd (DarcsInternalOption _) = Nothing
This is used when talking to GetOpt.
GetOpt doesn't need to know about the implicit options, so Nothing.
There's a stray 'wd' that GHC will warn us about likely
> setDefault :: Bool -> DarcsOption
> setDefault wantYes
> - | wantYes = mkMutuallyExclusive [] yes [no]
> - | otherwise = mkMutuallyExclusive [yes] no []
> + | wantYes = mkMutuallyExclusive [yes,no] defaultyes []
> + | otherwise = mkMutuallyExclusive [yes,no] defaultno []
> where
> hunk ./src/Darcs/Arguments.lhs 1112
> - yes = ( DarcsNoArgOption [] ["set-default"], SetDefault
> - , "set default repository" )
> - no = ( DarcsNoArgOption [] ["no-set-default"], NoSetDefault
> - , "don't set default repository" )
> + yes = ( DarcsNoArgOption [] ["set-default"], SetDefault True
> + , "set default repository" ++ defaultText wantYes )
> + no = ( DarcsNoArgOption [] ["no-set-default"], NoSetDefault True
> + , "don't set default repository" ++ defaultText (not wantYes) )
> + defaultyes = ( \f _ -> DarcsInternalOption f, SetDefault False, "" )
> + defaultno = ( \f _ -> DarcsInternalOption f, NoSetDefault False, "" )
> + defaultText True = " [DEFAULT]"
> + defaultText False = ""
The default option is now an implicit one.
Unfortunately, we lose the factorisation on the defaultText setting in
the process. Oh well. We could work harder on this if this wasn't an
expedient hack... but of course we also want to conserve our effort and
focus on getting the overhaul done instead.
> + -- The Bool parameters are a bit of a hack so that we can tell
> + -- whether the user explicitly set the option or not.
> + -- A more general mechanism would be better.
> + -- True = explicitly set by user (on command-line or in prefs/defaults),
> + -- False = defaulted by darcs
> + | SetDefault Bool | NoSetDefault Bool
COMMENT: maybe something like ExplicitlySet and ImplicitlySet would be
better, especially since you start running into double negations like
NoSetDefault False (speaking as the kind of person who's really prone
to confusion)
I was going to ask if we could have some kind of catch-all case
ImplicitDefault Flag, but thinking about it more, that wouldn't
work for code that has to test for existence of flag or otherwise
add instruction on suppressing set-default message
--------------------------------------------------
From a UI standpoint, I'd wonder if maybe just telling the user to
set ALL no-set-default would be better.
I figure it'd be surprising if you went through some procedure to
make darcs shut up (darcs get...) and then it doesn't (darcs pull).
Ah, the world of totally informal making-things-up-as-we-go-along
UI design without the backing of proper research. But even if we
were a well-resourced totally UI-obsessed team, there'd be limits
on what we can apply research on.
> - setDefaultrepo repodir opts
> + setDefaultrepo "get" repodir opts
Can't we just use commandName?
> -setDefaultrepo :: String -> [DarcsFlag] -> IO ()
> -setDefaultrepo r opts = do olddef <- getPreflist "defaultrepo"
> +setDefaultrepo :: String -> String -> [DarcsFlag] -> IO ()
> +setDefaultrepo command r opts = do
> + olddef <- getPreflist "defaultrepo"
> let doit = null noSetDefault && greenLight
> greenLight = wetRun
> && not rIsTmp
> hunk ./src/Darcs/Repository/Prefs.lhs 458
> && (olddef /= [r] || olddef == [])
> if doit
> then setPreflist "defaultrepo" [r]
> - else when (True `notElem` noSetDefault && greenLight) $ putStr . unlines $
> + else
> + when (True `notElem` noSetDefault && greenLight) $ do
> + mdir <- globalPrefsDir
> + putStr . unlines $
> -- the nuance here is that we should only notify when the
> -- reason we're not setting default is the --no-set-default
> -- flag, not the various automatic show stoppers
> hunk ./src/Darcs/Repository/Prefs.lhs 468
> [ "Note: if you want to change the default remote repository to"
> , r ++ ","
> , "quit now and issue the same command with the --set-default flag."
> - ]
> + ] ++
> + maybe [] (\dir -> [" To suppress this message globally, add \"" ++ command ++ " no-set-default\""
> + ," to " ++ dir </> "defaults"])
> + mdir
> addToPreflist "repos" r
> `catchall` return () -- we don't care if this fails!
> where
add test of set-default hint messages
-------------------------------------
Nice, very clear and readable test that checks both our expectation
(message triggers) and also for the unexpected (message does not
trigger when we're not expecting it). Hard to train ourselves to
check for the letter. Could maybe be part of the set-default test
script.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey at jabber.fr (Jabber or Google Talk only)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20110121/05a2d9a5/attachment.asc>
More information about the darcs-devel
mailing list