Re: [Changeset] new bzip2 function

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

John W. Eaton
Administrator
On  2-Nov-2008, Thorsten Meyer wrote:

| Hi,
|
| John W. Eaton wrote:
| > Probably you don't need that level of complexity.  I'm thinking of
| > something like
| >
| >   function entries = gzip (files, outdir)
| >     if (nargin == 1 || nargin == 2)
| >       __xzip__ (files, outdir, "gzip", ".gz", "gzip -r %s");
| >     else
| >       print_usage ();
| >     endif
| >   endfunction
| >
| > and
| >
| >   function entries = bzip2 (files, outdir)
| >     if (nargin == 1 || nargin == 2)
| >       __xzip__ (files, outdir, "bzip2", ".bz2", "bzip2 %s");
| >     else
| >       print_usage ();
| >     endif
| >   endfunction
| >
| > Given this, converting the current gzip function to __xzip__ should be
| > fairly easy, I think.
| Attached, you will find a new changeset, that implements the above.

Thank you for writing this patch and David for applying it.

+      error (sprintf("%s: extension has to be a string with finite length",
+                     commandname));

There's no need to use sprintf here since error already understands
format strings.  So you should write

  error ("%s: extension has to be a string with finite length", commandname);

jwe
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

Thorsten Meyer
Hi,

John W. Eaton wrote:
> +      error (sprintf("%s: extension has to be a string with finite length",
> +                     commandname));
>
> There's no need to use sprintf here since error already understands
> format strings.  So you should write
>
>   error ("%s: extension has to be a string with finite length", commandname);
>  
Thanks for the hint.

Attached, you will find a little patch which fixes the error messages in
__xzip__ and adds a few tests of the error handling.

regards

Thorsten

# HG changeset patch
# User Thorsten Meyer <[hidden email]>
# Date 1230805458 -3600
# Node ID 8f1edcf346b6e2f00b44becaffb62304ed53fab0
# Parent  628b6973b9fe1460dd88176f837d48bb39e07f3d
Fix error messages in __xzip__

diff -r 628b6973b9fe -r 8f1edcf346b6 scripts/ChangeLog
--- a/scripts/ChangeLog Mon Dec 29 11:21:42 2008 +0100
+++ b/scripts/ChangeLog Thu Jan 01 11:24:18 2009 +0100
@@ -0,0 +1,4 @@
+2009-01-01  Thorsten Meyer  <[hidden email]>
+
+ * miscellaneous/__xzip__.m: Fix error messages, add tests.
+
diff -r 628b6973b9fe -r 8f1edcf346b6 scripts/miscellaneous/__xzip__.m
--- a/scripts/miscellaneous/__xzip__.m Mon Dec 29 11:21:42 2008 +0100
+++ b/scripts/miscellaneous/__xzip__.m Thu Jan 01 11:24:18 2009 +0100
@@ -34,8 +34,7 @@
 
   if (nargin == 4 || nargin == 5)
     if (! ischar (extension) || length (extension) == 0)
-      error (sprintf("%s: extension has to be a string with finite length",
-                     commandname));
+      error ("__xzip__: extension has to be a string with finite length");
     endif
     
     if (nargin == 5 && ! exist (outdir, "dir"))
@@ -83,15 +82,15 @@
           ## FIXME this does not work when you try to compress directories
 
           compressed_files  = cellfun(@(x) sprintf ("%s.%s", x, extension),
-                            files, "UniformOutput", false);
+                                      files, "UniformOutput", false);
         endif
 
         if (nargout > 0)
           entries = compressed_files;
         endif
       else
-        error (sprintf("%s command failed with exit status = %d",
-                       commandname, status));
+        error ("%s command failed with exit status = %d",
+               commandname, status);
       endif
     unwind_protect_cleanup
       cd(cwd);
@@ -119,3 +118,22 @@
   d(idx) = "";
   f(idx) = files(idx);
 endfunction
+
+%!error <extension has to be a string with finite length>
+%!  __xzip__("gzip", "", "gzip -r %s", "bla");
+%!error <no files to move>
+%!  __xzip__("gzip", ".gz", "gzip -r %s", tmpnam);
+%!error <command failed with exit status>
+%!  # test __xzip__ with invalid compression command
+%!  unwind_protect
+%!    filename = tmpnam;
+%!    dummy    = 1;
+%!    save(filename, "dummy");
+%!    dirname  = tmpnam;
+%!    mkdir(dirname);
+%!    entry = __xzip__("gzip", ".gz", "xxxzipxxx -r %s 2>/dev/null",
+%!                     filename, dirname);
+%!  unwind_protect_cleanup
+%!    delete(filename);
+%!    rmdir(dirname);
+%!  end_unwind_protect
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

Jaroslav Hajek-2
On Thu, Jan 1, 2009 at 11:32 AM, Thorsten Meyer <[hidden email]> wrote:

> Hi,
>
> John W. Eaton wrote:
>> +      error (sprintf("%s: extension has to be a string with finite length",
>> +                     commandname));
>>
>> There's no need to use sprintf here since error already understands
>> format strings.  So you should write
>>
>>   error ("%s: extension has to be a string with finite length", commandname);
>>
> Thanks for the hint.
>
> Attached, you will find a little patch which fixes the error messages in
> __xzip__ and adds a few tests of the error handling.
>
> regards
>
> Thorsten
>

Applied.

thanks


--
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

John W. Eaton
Administrator
In reply to this post by Thorsten Meyer
On  1-Jan-2009, Thorsten Meyer wrote:

| John W. Eaton wrote:
| > +      error (sprintf("%s: extension has to be a string with finite length",
| > +                     commandname));
| >
| > There's no need to use sprintf here since error already understands
| > format strings.  So you should write
| >
| >   error ("%s: extension has to be a string with finite length", commandname);
| >  
| Thanks for the hint.
|
| Attached, you will find a little patch which fixes the error messages in
| __xzip__ and adds a few tests of the error handling.

| diff -r 628b6973b9fe -r 8f1edcf346b6 scripts/miscellaneous/__xzip__.m
| --- a/scripts/miscellaneous/__xzip__.m Mon Dec 29 11:21:42 2008 +0100
| +++ b/scripts/miscellaneous/__xzip__.m Thu Jan 01 11:24:18 2009 +0100
| @@ -34,8 +34,7 @@
|  
|    if (nargin == 4 || nargin == 5)
|      if (! ischar (extension) || length (extension) == 0)
| -      error (sprintf("%s: extension has to be a string with finite length",
| -                     commandname));
| +      error ("__xzip__: extension has to be a string with finite length");

Is it possible for these messages to be triggered by calling the bzip2
or gzip functions incorrectly?  If so, then the __xzip__ in the
messages won't help users much, and you should probably use the name
of the calling function in the error message.

Or, if these messages can only happen if __xzip__ itself is called
improperly, then I suppose it is OK to leave them as they are, or it
would also be OK to omit the checks entirely, since this is an
internal function.

jwe
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

Thorsten Meyer
John W. Eaton wrote:

> On  1-Jan-2009, Thorsten Meyer wrote:
>
> | John W. Eaton wrote:
> | > +      error (sprintf("%s: extension has to be a string with finite length",
> | > +                     commandname));
> | >
> | > There's no need to use sprintf here since error already understands
> | > format strings.  So you should write
> | >
> | >   error ("%s: extension has to be a string with finite length", commandname);
> | >  
> | Thanks for the hint.
> |
> | Attached, you will find a little patch which fixes the error messages in
> | __xzip__ and adds a few tests of the error handling.
>
> | diff -r 628b6973b9fe -r 8f1edcf346b6 scripts/miscellaneous/__xzip__.m
> | --- a/scripts/miscellaneous/__xzip__.m Mon Dec 29 11:21:42 2008 +0100
> | +++ b/scripts/miscellaneous/__xzip__.m Thu Jan 01 11:24:18 2009 +0100
> | @@ -34,8 +34,7 @@
> |  
> |    if (nargin == 4 || nargin == 5)
> |      if (! ischar (extension) || length (extension) == 0)
> | -      error (sprintf("%s: extension has to be a string with finite length",
> | -                     commandname));
> | +      error ("__xzip__: extension has to be a string with finite length");
>
> Is it possible for these messages to be triggered by calling the bzip2
> or gzip functions incorrectly?  If so, then the __xzip__ in the
> messages won't help users much, and you should probably use the name
> of the calling function in the error message.
>  
First, I had the error messages like you propose. Then I thought it
would be quite confusing to have an error message that says "gzip: some
error" followed by the error trace that gives "__xzip__ at line xx" and
then "gzip at line yy".  Thinking about it again, it is probably even
more confusing to get an error message that says "__xzip__: expecting
argument to be this and that"  when you actually have to change the
argument of the calling function.

Here is a list of other error messages in internal functions that do not
reference back to the caller (and do not explain, what the problem is on
the caller level) or do not use the "caller: " style:

scripts/general/__isequal__.m:    error ("__isequal__: list objects are
deprecated and cannot be tested for equality here; use cell arrays
instead");
scripts/general/__splinen__.m:    error ("Incorrect number of arguments");
scripts/general/__splinen__.m:    error ("%s: non gridded data or
dimensions inconsistent", f);
scripts/image/__img__.m:    error ("__img__: matrix is empty");
scripts/image/__img_via_file__.m:    error ("the image viewing command
failed");
scripts/plot/__add_datasource__.m:    error ("internal error");
scripts/plot/__errcomm__.m:    error ("wrong argument types");
scripts/plot/__errcomm__.m:    error ("argument sizes do not match");
scripts/plot/__errcomm__.m:    error ("too many arguments to a plot");
scripts/plot/__errplot__.m:    error ("error plot requires 2, 3, 4 or 6
columns");
scripts/plot/__errplot__.m:    error ("error plot requires 2, 3, 4 or 6
columns");
scripts/plot/__gnuplot_ginput__.m:    error ("ginput: stream to gnuplot
not open");
scripts/plot/__go_draw_axes__.m:          error ("__go_draw_axes__:
invalid grid data");
scripts/plot/__go_draw_axes__.m:      error ("__go_draw_axes__: unknown
object class, %s",
scripts/plot/__go_draw_axes__.m:    error ("unsupported type of ticklabel");
scripts/plot/__go_draw_figure__.m:          error ("__go_draw_figure__:
unknown object class, %s",
scripts/plot/__go_draw_figure__.m:      error ("__go_draw_figure__:
expecting figure object, found `%s'",
scripts/plot/__next_line_color__.m:      error ("__next_line_color__:
color_rotation not initialized");
scripts/plot/__patch__.m:    error ("patch: not supported");
scripts/plot/__patch__.m:    error ("patch: color value not valid");
scripts/plot/__patch__.m:    error ("patch: size of x, y, and c must be
equal")
scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting");
scripts/plot/__plr2__.m:        error ("__plr2__: vector lengths must
match");
scripts/plot/__plr2__.m:        error ("__plr2__: vector and matrix
sizes must match");
scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting")
scripts/plot/__plr2__.m:        error ("__plr2__: vector and matrix
sizes must match");
scripts/plot/__plr2__.m:        error ("__plr2__: matrix dimensions must
match");
scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting")
scripts/plot/__plr2__.m:    error ("__plr2__: invalid data for plotting")
scripts/plot/__plt1__.m:    error ("__plt1__: options must be a struct
array");
scripts/plot/__plt2__.m:    error ("__plt1__: options must be a struct
array");
scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
scripts/plot/__plt2__.m:    error ("__plt2__: invalid data for plotting");
scripts/plot/__plt2mm__.m:      error ("__plt2mm__: arguments must be a
matrices");
scripts/plot/__plt2mm__.m:    error ("__plt2mm__: matrix dimensions must
match");
scripts/plot/__plt2mv__.m:    error ("__plt2mv__: matrix dimensions must
match");
scripts/plot/__plt2mv__.m:    error ("__plt2mv__: arguments must be a
matrices");
scripts/plot/__plt2ss__.m:    error ("__plt2ss__: arguments must be
scalars");
scripts/plot/__plt2sv__.m:    error ("__plt2sv__: first arg must be
scalar, second arg must be vector");
scripts/plot/__plt2vm__.m:    error ("__plt2vm__: matrix dimensions must
match");
scripts/plot/__plt2vm__.m:    error ("__plt2vm__: arguments must be a
matrices");
scripts/plot/__plt2vs__.m:    error ("__plt2vs__: first arg must be
vector, second arg must be scalar");
scripts/plot/__plt2vv__.m:    error ("__plt2vv__: vector lengths must
match");
scripts/plot/__plt__.m:      error ("plot: no data to plot");
scripts/plot/__pltopt__.m:      error ("__pltopt__: expecting argument
to be character string or cell array of character strings");

Should those also be fixed?
> Or, if these messages can only happen if __xzip__ itself is called
> improperly, then I suppose it is OK to leave them as they are, or it
> would also be OK to omit the checks entirely, since this is an
> internal function.
>  
The one check for correct call of __xzip__ itself is to prevent having
equal source and target file name which may lead to a loss of data in
some cases. I would rather leave that in.

Regards

Thorsten
Reply | Threaded
Open this post in threaded view
|

Re: [Changeset] new bzip2 function

John W. Eaton
Administrator
On  5-Jan-2009, Thorsten Meyer wrote:

| First, I had the error messages like you propose. Then I thought it
| would be quite confusing to have an error message that says "gzip: some
| error" followed by the error trace that gives "__xzip__ at line xx" and
| then "gzip at line yy".  Thinking about it again, it is probably even
| more confusing to get an error message that says "__xzip__: expecting
| argument to be this and that"  when you actually have to change the
| argument of the calling function.
|
| Here is a list of other error messages in internal functions that do not
| reference back to the caller (and do not explain, what the problem is on
| the caller level) or do not use the "caller: " style:
|
| scripts/general/__isequal__.m:    error ("__isequal__: list objects are
| deprecated and cannot be tested for equality here; use cell arrays
| instead");
| scripts/general/__splinen__.m:    error ("Incorrect number of arguments");
| scripts/general/__splinen__.m:    error ("%s: non gridded data or
| dimensions inconsistent", f);
| scripts/image/__img__.m:    error ("__img__: matrix is empty");
| scripts/image/__img_via_file__.m:    error ("the image viewing command
| failed");
| scripts/plot/__add_datasource__.m:    error ("internal error");
| scripts/plot/__errcomm__.m:    error ("wrong argument types");
| scripts/plot/__errcomm__.m:    error ("argument sizes do not match");
| scripts/plot/__errcomm__.m:    error ("too many arguments to a plot");
| scripts/plot/__errplot__.m:    error ("error plot requires 2, 3, 4 or 6
| columns");
| scripts/plot/__errplot__.m:    error ("error plot requires 2, 3, 4 or 6
| columns");
| scripts/plot/__gnuplot_ginput__.m:    error ("ginput: stream to gnuplot
| not open");
| scripts/plot/__go_draw_axes__.m:          error ("__go_draw_axes__:
| invalid grid data");
| scripts/plot/__go_draw_axes__.m:      error ("__go_draw_axes__: unknown
| object class, %s",
| scripts/plot/__go_draw_axes__.m:    error ("unsupported type of ticklabel");
| scripts/plot/__go_draw_figure__.m:          error ("__go_draw_figure__:
| unknown object class, %s",
| scripts/plot/__go_draw_figure__.m:      error ("__go_draw_figure__:
| expecting figure object, found `%s'",
| scripts/plot/__next_line_color__.m:      error ("__next_line_color__:
| color_rotation not initialized");
| scripts/plot/__patch__.m:    error ("patch: not supported");
| scripts/plot/__patch__.m:    error ("patch: color value not valid");
| scripts/plot/__patch__.m:    error ("patch: size of x, y, and c must be
| equal")
| scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting");
| scripts/plot/__plr2__.m:        error ("__plr2__: vector lengths must
| match");
| scripts/plot/__plr2__.m:        error ("__plr2__: vector and matrix
| sizes must match");
| scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting")
| scripts/plot/__plr2__.m:        error ("__plr2__: vector and matrix
| sizes must match");
| scripts/plot/__plr2__.m:        error ("__plr2__: matrix dimensions must
| match");
| scripts/plot/__plr2__.m:      error ("__plr2__: invalid data for plotting")
| scripts/plot/__plr2__.m:    error ("__plr2__: invalid data for plotting")
| scripts/plot/__plt1__.m:    error ("__plt1__: options must be a struct
| array");
| scripts/plot/__plt2__.m:    error ("__plt1__: options must be a struct
| array");
| scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
| scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
| scripts/plot/__plt2__.m:      error ("__plt2__: invalid data for plotting");
| scripts/plot/__plt2__.m:    error ("__plt2__: invalid data for plotting");
| scripts/plot/__plt2mm__.m:      error ("__plt2mm__: arguments must be a
| matrices");
| scripts/plot/__plt2mm__.m:    error ("__plt2mm__: matrix dimensions must
| match");
| scripts/plot/__plt2mv__.m:    error ("__plt2mv__: matrix dimensions must
| match");
| scripts/plot/__plt2mv__.m:    error ("__plt2mv__: arguments must be a
| matrices");
| scripts/plot/__plt2ss__.m:    error ("__plt2ss__: arguments must be
| scalars");
| scripts/plot/__plt2sv__.m:    error ("__plt2sv__: first arg must be
| scalar, second arg must be vector");
| scripts/plot/__plt2vm__.m:    error ("__plt2vm__: matrix dimensions must
| match");
| scripts/plot/__plt2vm__.m:    error ("__plt2vm__: arguments must be a
| matrices");
| scripts/plot/__plt2vs__.m:    error ("__plt2vs__: first arg must be
| vector, second arg must be scalar");
| scripts/plot/__plt2vv__.m:    error ("__plt2vv__: vector lengths must
| match");
| scripts/plot/__plt__.m:      error ("plot: no data to plot");
| scripts/plot/__pltopt__.m:      error ("__pltopt__: expecting argument
| to be character string or cell array of character strings");
|
| Should those also be fixed?

I think they can be left as is if the only way these messages can be
triggered is when there is some internal usage that is wrong, because
we don't expect that to happen and if it does, there is a bug to fix
in Octave.

But if an incorrect user input to plot (for example) can cause an
error in __plt2mm__, then we should probably pass the name of the
calling function in to these internal functions and use that in the
messages.

I don't see this as a high priority project, but it would be nice to
fix if someone has time.

| The one check for correct call of __xzip__ itself is to prevent having
| equal source and target file name which may lead to a loss of data in
| some cases. I would rather leave that in.

OK.

Thanks,

jwe