isempty bug fix - Githubissues

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

isempty bug fix #90

Closed rdzman closed 2 years ago

rdzman commented 2 years ago

Fix bug where isempty(table([])) returned 0.

The isempty() method was not checking the number of rows in a table with a non-zero number of columns.

apjanke commented 2 years ago

smacks forehead. Oh my gosh, what an oversight! Thanks for catching this.

Two minor things:

Needs a space before parentheses in function calls to conform to GNU coding style.

  out = isempty (this.VariableValues) || size (this.VariableValues{1}, 1);

How about doing a size (...) == 0 instead of just letting the size value be coerced to a logical by the ||? I believe it will produce the exact same result, but seems a bit more readable to me.

  out = isempty (this.VariableValues) || (size (this.VariableValues{1}, 1) == 0);

apjanke commented 2 years ago

This is a big enough bug that I think it justifies making a point release once it's fixed. I'll look at rolling out a Tablicious 0.3.7 release.

rdzman commented 2 years ago

The following is an alternative that should be equivalent and potentially more readable, but possibly a little less efficient ...

out = prod (size (this) ) == 0;

apjanke commented 2 years ago

Ooh, I like that prod (size ...) alternative! Seems more readable to me, and the efficiency difference should be negligible.

There may be subtleties with regards to the fact that table overrides size, but I think it should work. Just make sure to actually test it first if you want to go that way.

rdzman commented 2 years ago

I don't really have a preference, and prod (size ...) does work. I'm happy to change it to that if you prefer.

apjanke commented 2 years ago

Yeah, let's do prod (size ...). I think that is a better design, and more readable.

apjanke commented 2 years ago

Looking good!

Couple minor things:

No space before the closing paren in a function call: prod (size (this) ) should be prod (size (this))
Can you squash your commits to a single commit, to keep the git history tidy? If I do it for you with a squash-merge in GitHub, I think you might lose authorship credit.

rdzman commented 2 years ago

Ok, I think it should be ready now. Thanks for your patience with the style issues.

apjanke commented 2 years ago

Yes, looking good! And thank you for being willing to do the style fixes.

I will do testing this evening and then pull it in.

apjanke commented 2 years ago

Merged! New release with this coming up in a few days, hopefully.