apjanke / octave-tablicious

Table (relational, tabular data) implementation for GNU Octave
https://apjanke.github.io/octave-tablicious/
GNU General Public License v3.0
29 stars 11 forks source link

"[ ... fcn (...) ...]" syntax problem in mycombvec, datetime, etc. #127

Open apjanke opened 8 months ago

apjanke commented 8 months ago

As reported by @mmuetzel in https://github.com/gnu-octave/packages/pull/401 (my paraphrasing):


One of the doctests is failing:

      message = parse error near line 48 of file /usr/share/octave/packages/tablicious-0.4.2/+tblish/+internal/mycombvec.m
    syntax error
  >>>         out = [out; [repmat (a(i), [size (rest_combs,1) 1]) rest_combs]];
                                        ^
[...]

Spaces (`) separate elements inside brackets ([ ]`). So, that line is interpreted as out = [out; [repmat, (a(i), [size, (rest_combs,1), 1]), rest_combs]];. Combining comma-separated lists with parenthesis is not supported (and is probably not what you wanted).

That line should probably be the following instead:

out = [out; [repmat(a(i), [size(rest_combs,1), 1]), rest_combs]];

My notes:

Those extra spaces were a mistake. Probably introduced recently when I did this "Convert to GNU Octave code style" change, where I added a space at function calls to be "foo (...)" instead of "foo()", but didn't catch by manual inspection the places where that wasn't correct.

Probably introduced in v0.4.0.

This affected a few other functions, too. And in some cases, they were single-arg function calls, so no commas in the "(...)", which I think means they weren't syntax errors, but were incorrectly calling the function with zero args, and then concatenating its return value with the "(...)" expression that was supposed to be the function args, maybe raising a run-time error or just producing incorrect results. That's worse.

I've checked in a hopeful fix in https://github.com/apjanke/octave-tablicious/commit/151d2eca32f43ced25a6b84c71958a9f61ed692a and will be merging it soon and rolling a corrective patch release. Hopefully get some more unit test coverage in there too.

Recording this bug as its own ticket for reference, even though it's almost (I think) fixed.

apjanke commented 3 months ago

Moved from milestone 0.4.3 to 0.4.4, because I want to push out 0.4.3 now to pick up https://github.com/apjanke/octave-tablicious/issues/134.