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

Trent W. Buck trentbuck at gmail.com
Mon Feb 2 09:31:39 UTC 2009


On Mon, Feb 02, 2009 at 07:53:07AM +0000, Eric Kow wrote:
> 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

Or, since this test just causes things to be printed at present, just
using several `when`s in a single do block.

> > +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)

Note that prior to this patch, it *does* prompt to continue (unless
--all is present) for the leading-hyphen case.  I stripped that
functionality out for now.

>, 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?

That's what I'm trying to achieve, but I don't see how I could be any
*less* obtrusive.

> > +                     -- 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?

Well, that's where more testing than I did comes into play :-)

Also, this will only apply if n is a linked list of Unicode codepoints
(not a byte string).  I'm not sure if that's properly the case for
terminal input in GHC 6.8 and earlier.

> 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

I welcome style suggestions :-)

The null would hopefully be caught by the earlier length n < 10.

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

Well, that's why it's a warning instead of an error.  I wanted to give
helpful hints to newbies (and tired Darcs developers!) so that they
don't end up with a prestigious project that has a bunch of stupid /
ugly change descriptions in the early (and thus unamendable) history.

Under what circumstances is it desirable for a patch name to *not* be
a sentence?


More information about the darcs-users mailing list