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 <[email protected]>
> > * Resolve issue1093: warn about ugly patch names.
> >
> > Mon Feb 2 13:51:14 EST 2009 Trent W. Buck <[email protected]>
> > * 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?
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
