You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <e....@gmx.net> on 2003/11/23 12:14:03 UTC
Error message conventions
I have tried to describe the changes I made to the error messages I have
done for issue 897 so far. The patch below is an addition for the HACKING file,
but that is beside the point for now. It is not a patch submission yet.
Rather I would like to discuss if you agree with what it says. I'm interested in
additions, but also incorrectness or just disagreement.
The patch below tries to describe the conventions I try to adhere to when
changing error messages for issue 897; it can also be found at
http://encorps.dnsalias.com/svn-public/patches/HACKING.patch.
Here is the not-a-patch-submission-yet:
Index: HACKING
===================================================================
--- HACKING (revision 7765)
+++ HACKING (working copy)
@@ -19,6 +19,7 @@
* Secure coding guidelines
* Document everything
* Using page breaks
+ * Error message conventions
* Other conventions
* APR pool usage conventions
* APR status codes
@@ -371,6 +372,44 @@
+Error message conventions
+=========================
+
+For error messages the following conventions apply:
+
+ * Provide specific error messages only when there is information
+ to add to the general error message found in
+ subversion/include/svn_error_codes.h
+
+ * Messages start with a capital letter.
+
+ * Keep messages below 70 characters.
+
+ * Don't end the error message with a '.'.
+
+ * Don't include newline characters in error messages.
+
+ * Quoting information is done using single quotes ('some info').
+
+ * Don't include the name of the function where the error occurs
+ in the error message. If subversion is compiled using the
+ '--enable-maintainer-mode' configure-flag, if will provide this
+ information by itself.
+
+ * When including path or filenames in the error string, be sure
+ to quote them. (i.e. "Can't find '/path/to/repos/userfile'")
+
+ * If you want to add an explanation to the error, report the error,
+ followed by a colon and the explanation like this:
+ "Invalid " SVN_PROP_EXTERNALS " property on '%s': "
+ "target involves '.' or '..'"
+
+ * Suggestions or other additions can be added after a semi-colon,
+ like this:
+ "Can't write to '%s': object of same name already exists; remove "
+ "before retrying"
+
+
Other conventions
=================
--
GMX Weihnachts-Special: Seychellen-Traumreise zu gewinnen!
Rentier entlaufen. Finden Sie Rudolph! Als Belohnung winken
tolle Preise. http://www.gmx.net/de/cgi/specialmail/
+++ GMX - die erste Adresse f�r Mail, Message, More! +++
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by kf...@collab.net.
Mike Mason <mg...@thoughtworks.net> writes:
> Is it useful to line up the checksums for visual comparison? If two
> MD5s are different, they're usually completely different rather than
> differing by one or two characters, and lining them up on screen
> doesn't help much.
No, it's not useful in that way -- I just meant that it makes better
visual presentation, because it makes it obvious that the two things
are meant to be compared (no matter how many chars they differ by).
However, I'm convinced by Greg's and others' arguments that we should
avoid newlines, and am happy to pay the price of horizontally listed
checksums, so please ignore my objection anyway.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by Mike Mason <mg...@thoughtworks.net>.
Greg Hudson wrote:
>On Sun, 2003-11-23 at 19:39, Branko Čibej wrote:
>
>
>>kfogel@collab.net wrote:
>>
>>
>>>Sometimes it's better, like with checksum mismatch error messages that
>>>compare actual vs expected checksum -- you want the two checksums to
>>>line up vertically. For example this code from reps-strings.c:
>>>
>>>
>
>Note that the current error message only lines up the checksums with a
>fixed-width font.
>
>
Is it useful to line up the checksums for visual comparison? If two MD5s
are different, they're usually completely different rather than
differing by one or two characters, and lining them up on screen doesn't
help much.
Mike.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2003-11-23 at 19:39, Branko Čibej wrote:
> kfogel@collab.net wrote:
> >Sometimes it's better, like with checksum mismatch error messages that
> >compare actual vs expected checksum -- you want the two checksums to
> >line up vertically. For example this code from reps-strings.c:
Note that the current error message only lines up the checksums with a
fixed-width font.
> This is one of the bits of error handling code we discussed.
>
> I argued against newlines in favour of using svn_error_t chaining to
> break out the separate lines, because that's much friendlier to GUI clients.
>
> Greg argued against both newlines and svn_error_t chaining, on the
> grounds that the lines belong to the same error message, and therefore
> should not be wrapped in a lower-level error..
I think the issue of multi-line error messages is a tough one, and we
should avoid it by sticking to single-line error messages for now.
Branko also wrote:
> >+ * Provide specific error messages only when there is information
> >+ to add to the general error message found in
> >+ subversion/include/svn_error_codes.h
> If we enforce this, then we must allow parameter placeholders in those
> messages. Otherwise people can't both use svn_error_createf and obey
> this guideline.
There must be some misunderstanding here.
My belief is: if you're returning something like SVN_ERR_XML_MALFORMED,
and you have nothing to add to the generic error message ("XML data was
not well-formed"), you should provide NULL as the error message, and the
error handler will use the generic message. On the other hand, let's
say we want to show what file and line the malformed XML data was on; in
this case, you should provide a message that stands on its own, like:
XML data was not well-formed in file '%s', line '%d'
and the error handler should display just this message, and not the
generic one.
Is it bad that we are replicating the default message here? I don't
think so. For one thing, if we vary the wording differently, it doesn't
actually hurt. (Phillip complained that the user would have no
immediate way to identify that two error messages share the same error
code, but who cares? Error codes are for programs, not for humans, and
if you're grepping for an error message, you're actually going to have
an easier time if it's more unique, not less unique.) For another,
adding parameter placeholders to the generic error messages would be
over-engineering.
Am I clear?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>"Erik Huelsmann" <e....@gmx.net> writes:
>
>
>>The patch below tries to describe the conventions I try to adhere to when
>>changing error messages for issue 897; it can also be found at
>>http://encorps.dnsalias.com/svn-public/patches/HACKING.patch.
>>
>>
>
>Thanks for writing this up -- much needed!
>
>Comments below. Mostly I agree with your suggested conventions, except
>for one:
>
>
>
>>+ * Don't include newline characters in error messages.
>>
>>
>
>Sometimes it's better, like with checksum mismatch error messages that
>compare actual vs expected checksum -- you want the two checksums to
>line up vertically. For example this code from reps-strings.c:
>
>
This is one of the bits of error handling code we discussed.
I argued against newlines in favour of using svn_error_t chaining to
break out the separate lines, because that's much friendlier to GUI clients.
Greg argued against both newlines and svn_error_t chaining, on the
grounds that the lines belong to the same error message, and therefore
should not be wrapped in a lower-level error..
> if (! svn_md5_digests_match (checksum, rep->checksum))
> return svn_error_createf
> (SVN_ERR_FS_CORRUPT, NULL,
> "svn_fs__rep_contents: checksum mismatch on rep \"%s\":\n"
> " expected: %s\n"
> " actual: %s\n", rep_key,
> svn_md5_digest_to_cstring (rep->checksum, trail->pool),
> svn_md5_digest_to_cstring (checksum, trail->pool));
>
>So maybe the guideline should be "Try not to" instead of "Don't" ?
>
>
I still think I'm right, and I agree with Erik and Greg that newlines
have no place in the error messages. We should _not_ force clients to
parse the error messages or reformat the lines. If that means we need an
extra mechanism in addition to chaining to construct multilined
messages, then so be it.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by kf...@collab.net.
"Erik Huelsmann" <e....@gmx.net> writes:
> The patch below tries to describe the conventions I try to adhere to when
> changing error messages for issue 897; it can also be found at
> http://encorps.dnsalias.com/svn-public/patches/HACKING.patch.
Thanks for writing this up -- much needed!
Comments below. Mostly I agree with your suggested conventions, except
for one:
> + * Don't include newline characters in error messages.
Sometimes it's better, like with checksum mismatch error messages that
compare actual vs expected checksum -- you want the two checksums to
line up vertically. For example this code from reps-strings.c:
if (! svn_md5_digests_match (checksum, rep->checksum))
return svn_error_createf
(SVN_ERR_FS_CORRUPT, NULL,
"svn_fs__rep_contents: checksum mismatch on rep \"%s\":\n"
" expected: %s\n"
" actual: %s\n", rep_key,
svn_md5_digest_to_cstring (rep->checksum, trail->pool),
svn_md5_digest_to_cstring (checksum, trail->pool));
So maybe the guideline should be "Try not to" instead of "Don't" ?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Error message conventions
Posted by Branko Čibej <br...@xbc.nu>.
Erik Huelsmann wrote:
>+Error message conventions
>+=========================
>+
>+For error messages the following conventions apply:
>+
>+ * Provide specific error messages only when there is information
>+ to add to the general error message found in
>+ subversion/include/svn_error_codes.h
>
>
If we enforce this, then we must allow parameter placeholders in those
messages. Otherwise people can't both use svn_error_createf and obey
this guideline.
>+ * Keep messages below 70 characters.
>
>
This is hard to do in practice, once you take interpolated file names
into account. But I agree in principle.
>+ * When including path or filenames in the error string, be sure
>+ to quote them. (i.e. "Can't find '/path/to/repos/userfile'")
>
>
And convert the file names with svn_path_local_style before
interpolating them into the error message text.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org