theory / svn-notify

Subversion activity notification
http://search.cpan.org/dist/SVN-Notify/
18 stars 18 forks source link

Allow long lines in log messages to wrap #8

Closed dabrahams closed 11 years ago

theory commented 11 years ago

This is what the --wrap-log option is for.

dabrahams commented 11 years ago

Gee… I’m not sure how or whether this works for me (I’m not much of a Perl programmer), but maybe it would be a good idea to put it in the “—help” output. If it had been there, I might never have wasted time on this patch.

theory commented 11 years ago

Yeah, I admit it isn't always easy to trace the path to the handler-specific options. The key is that, if you are loading the HTML handler, the docs suggest you read the handler class's docs. And of course, if you are not a Perl programmer, it may not be intuitive to know how to find them. But they are there.

I would not write the docs the same way if I were doing this again. :-)

theory commented 11 years ago

BTW, you can see the docs for you handler with man SVN::Notify::HTML and, if you're using ColorDiff, man SVN::Notify::HTML::ColorDiff.

dabrahams commented 11 years ago

on Mon Jul 29 2013, "David E. Wheeler" wrote:

BTW, you can see the docs for you handler with man SVN::Notify::HTML and, if you're using ColorDiff, man SVN::Notify::HTML::ColorDiff.

Given that this option is almost always what you want, IMO it shouldn't be buried that way.

Cheers,

Dave Abrahams

theory commented 11 years ago

The way documentation is organized leaves a lot to be desired. However, it s only what you want if you are using the HTML (or HTML::ColorDiff) classes. The docs always point out that if you are using a handler, you should read its docs, too.

dabrahams commented 11 years ago

Actually, after using --wrap-log for a while, I discovered it doesn’t do what we want: it should preserve any existing line breaks in the log message. What if someone embeds code there, or, heck, just inserts a bullet list? My patch does exactly what we want: simply wraps lines that are too long. Again I submit that this should be the default behavior, and nobody should have to go digging for it in the documentation.