[darcs-users] darcs patch: Resolve issue1093: warn about ugly patch... (and 1 more)

[Eric Kow]

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

 diagnostics =
  [ 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
way?

> +                     -- 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.
--------------------------------------------
Thanks

-- 
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
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20090202/6efa6291/attachment.pgp