[darcs-users] darcs patch: Resolve issue1093: warn about ugly patch... (and 1 more)
kowey at darcs.net
Mon Feb 2 07:53:07 UTC 2009
Hmm... I'm a bit hesitant. See below.
On Sun, Feb 01, 2009 at 18:50:30 -0800, Trent W. Buck wrote:
> I'm sick of this sitting on my hard disk, and I want it applied to
> trunk. So I resolved conflicts with trunk and am sending it.
> Mon Feb 2 13:41:46 EST 2009 Trent W. Buck <trentbuck at gmail.com>
> * Resolve issue1093: warn about ugly patch names.
> Mon Feb 2 13:51:14 EST 2009 Trent W. Buck <trentbuck at gmail.com>
> * Test issue1093: warn about ugly patch names.
Resolve issue1093: warn about ugly patch names.
> +-- FIXME: should print *all* matching style warnings.
Not that you were asking for suggestions on how to go about doing this,
but I wonder if this would be a suitable approach
diagnostic match w n = if match n then Just w else Nothing
[ diagnostic (\n -> length n < 10) "WARNING: ..."
, diagnostic (\n -> length n > 70) "WARNING: ..."
mapMaybe ($ n) diagnostics
> +check_name_is_not_ugly :: [DarcsFlag] -> IO ()
> +check_name_is_not_ugly opts = when (length patchNames == 1) $ do diagnose (head patchNames)
> + where patchNames = [n | PatchName n <- opts]
> + diagnose :: String -> IO ()
> + diagnose n@('-':_) =
> + putStrLn ("WARNING: the patch name \"" ++ n ++ "\" looks like an option.") >>
> + putStrLn "This is discouraged because it may confuse command-line utilities." >>
> + amend
> + diagnose n | length n < 10 = -- this number is totally arbitrary
> + putStrLn "WARNING: the patch name is very short." >>
> + putStrLn "Does it convey an adequate summary to other contributors?" >>
> + amend
If we were to prompt to amend (I don't think we should by the way, but
that's just a gut feeling, best to observe users to find out), this
could become very annoying very fast. I love that darcs is friendly and
helpful, but I am also a bit afraid of crossing the line into being
officious. Can we find a way to help while staying politely out of the
> + -- Note that we use isLower here instead of not
> + -- . isUpper because some languages are caseless.
> + | isLower $ head n =
Your comment is interesting. So I presume this means that in caseless
languages, isUpper is always True and isLower is always False? Also,
this use of head could explode on us, as I don't see us having trapped
the null case anywhere. Seems that we could do this just as well with
a pattern match
> + | not $ isPunctuation $ last n =
> + putStrLn "WARNING: the patch name doesn't end with punctuation." >>
> + putStrLn "Using a whole sentence for each patch name is recommended." >>
Again, kaboom warning (last) and also imposing-our-preferences-on-others
warning. It's one thing if we're catching patch names that start with
'-' because, very likely, the user did not intend to write such a patch
name. It's quite another thing if we start telling users how they
should want to write patch names.
Test issue1093: warn about ugly patch names.
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 194 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20090202/6efa6291/attachment.pgp
More information about the darcs-users