Incorrect usage shouldn't give a long help message [was: [PATCH] Move multiple targets]

[Julian Foad <ju...@btopenworld.com>]

Hyrum K. Wright wrote:
> 
> Something else I noticed is that when svn mv is run without arguments,
> it prints the command line help, but when something like svn propset is
> run, it just errors with 'Too few arguments'.  I suspect there are other
> commands like this.  Should their behavior be changed to be consistent
> with move, or should move just error out?  Personally, I think that the
> former idea a better help for the user.

(I'm replying to this part of your mail first, because it's easy to do so.)

Printing help may be more helpful to a novice in the short term, but in the 
long term it gives a false sense of security, causing users to develop a bad 
habit.  It's bad because not all commands print help when invoked without 
arguments.  Also, when invoking the command accidentally without arguments 
(perhaps giving as an argument an environment variable which was accidentally 
undefined), printing a long help message is annoying.  Also it makes a 
compatibility problem if we want to extend a command to have an argument-free 
form in the future.

Incorrect usage should result in an error message, consistently for all 
commands.  It's been on my "to do" list for years.  Perhaps now I'll do it.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Re: Incorrect usage shouldn't give a long help message

[Julian Foad <ju...@btopenworld.com>]

Hyrum K. Wright wrote:
> Julian Foad wrote:
> 
>>Incorrect usage should result in an error message, consistently for all
>>commands.  It's been on my "to do" list for years.  Perhaps now I'll do it.
> 
> Sounds reasonable.  Here's a patch to print an error instead of the
> online help for the move command.
> 
> -Hyrum
> 
> [[[
>   * subversion/clients/cmdline/move-cmd.c
>     (svn_cl__move): Change the error from NULL to something useful, thus
>     preventing the display of the online help when there aren't enough
>     arguments.
[...]
> -    return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
> +    return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, 
> +                             _("No source and/or destination path given"));

Well, that's one way to do it.  I was thinking, instead, of removing all the 
code that prints extra help, as in the attached patch.  (My favourite type of 
programming: removing needless complexity. :-)

The only concerns I have with my patch are (1) some people are likely to be 
uncomfortable with the whole idea, so we need a little time for consultation; 
and (2) some of the error messages that will now appear may be not as good as 
we would wish, but I think we can improve them separately.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Re: Incorrect usage shouldn't give a long help message

[Julian Foad <ju...@btopenworld.com>]

Julian Foad wrote:
> 
> On argument-parsing errors, don't print help, just an error message.
> The errors in question are mainly for too few or too many arguments.

Committed in 17349.

> 
> Printing help may be more helpful to a novice in the short term, but in the
> long term it gives a false sense of security, causing users to develop a bad
> habit.  It's bad because not all commands print help when invoked without
> arguments.  Also, when invoking the command accidentally without arguments
> (perhaps giving as an argument an environment variable which was accidentally
> undefined), printing a long help message is annoying.  Also it makes a
> compatibility problem if we want to extend a command to have an argument-free
> form in the future.
> 
> * subversion/clients/cmdline/main.c (main):
>   Don't replace a generic argument-parsing error with help.
> 
> * subversion/clients/cmdline/merge-cmd.c (svn_cl__merge):
> * subversion/svnadmin/main.c (main):
> * subversion/svndumpfilter/main.c (main):
> * subversion/svnlook/main.c (main):
>   Don't augment argument-parsing errors with help.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Re: Incorrect usage shouldn't give a long help message

[Julian Foad <ju...@btopenworld.com>]

(Re-sent, this time with the patch.)

Hyrum K. Wright wrote:
> Julian Foad wrote:
> 
>>Incorrect usage should result in an error message, consistently for all
>>commands.  It's been on my "to do" list for years.  Perhaps now I'll do it.
> 
> Sounds reasonable.  Here's a patch to print an error instead of the
> online help for the move command.
> 
> -Hyrum
> 
> [[[
>   * subversion/clients/cmdline/move-cmd.c
>     (svn_cl__move): Change the error from NULL to something useful, thus
>     preventing the display of the online help when there aren't enough
>     arguments.
[...]
> -    return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, NULL);
> +    return svn_error_create (SVN_ERR_CL_ARG_PARSING_ERROR, 0, 
> +                             _("No source and/or destination path given"));

Well, that's one way to do it.  I was thinking, instead, of removing all the
code that prints extra help, as in the attached patch.  (My favourite type of
programming: removing needless complexity. :-)

The only concerns I have with my patch are (1) some people are likely to be
uncomfortable with the whole idea, so we need a little time for consultation;
and (2) some of the error messages that will now appear may be not as good as
we would wish, but I think we can improve them separately.

- Julian

[PATCH] Re: Incorrect usage shouldn't give a long help message

["Hyrum K. Wright" <hy...@byu.edu>]

Julian Foad wrote:
> Incorrect usage should result in an error message, consistently for all
> commands.  It's been on my "to do" list for years.  Perhaps now I'll do it.

Sounds reasonable.  Here's a patch to print an error instead of the
online help for the move command.

-Hyrum

[[[
  * subversion/clients/cmdline/move-cmd.c
    (svn_cl__move): Change the error from NULL to something useful, thus
    preventing the display of the online help when there aren't enough
    arguments.
]]]