apjanke / octave-tablicious

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

Fix bug when installing in GNU Octave v7.1.0 #95

Closed gkourachanis closed 1 year ago

gkourachanis commented 2 years ago

Fixes #91

Some functions' names have had their name changed in "libinterp/corefcn/event-manager.cc" of the GNU Octave source code.

I've initially found out about the issue here (after facing the problem myself, as well) : https://octave.discourse.group/t/problem-installing-tablicious-package-on-octave-7-1-0/2683/2

carlosal1015 commented 2 years ago

I think that is not working over Arch Linux. image

octave:1> pkg install https://github.com/gkourachanis/octave-tablicious/archive/master.tar.gz
octave:2> pkg load tablicious
gkourachanis commented 2 years ago

I think that is not working over Arch Linux. image

octave:1> pkg install https://github.com/gkourachanis/octave-tablicious/archive/master.tar.gz
octave:2> pkg load tablicious

Hey, thanks for checking it. There was an 'endif' missing and I've added it with the latest commit. Feel free to test again.

carlosal1015 commented 2 years ago

Now is working, few warnings, no errors anymore with

Name            : octave
Version         : 7.1.0-4
Description     : A high-level language, primarily intended for numerical computations
Architecture    : x86_64
URL             : https://www.gnu.org/software/octave/
Licenses        : GPL
Groups          : None
Provides        : None
Depends On      : fftw  curl  graphicsmagick  glpk  hdf5  qhull  arpack  glu  ghostscript  sundials  gl2ps  qscintilla-qt5  libsndfile  qt5-tools  qrupdate
Optional Deps   : texinfo: for help-support in octave [installed]
                  gnuplot: alternative plotting
                  portaudio: audio support
                  java-runtime: java support
                  fltk: FLTK GUI
                  texlive-bin: for the publish command
Required By     : None
Optional For    : None
Conflicts With  : None
Replaces        : None
Installed Size  : 62.29 MiB
Packager        : Antonio Rojas <arojas@archlinux.org>
Build Date      : Fri May 20 12:40:37 2022
Install Date    : Sat Jul 9 17:06:29 2022
Install Reason  : Explicitly installed
Install Script  : No
Validated By    : Signature

image

apjanke commented 2 years ago

Code is looking good. I tested this under Octave 7.1.0 on macOS, and it loads up no problem, and the doc command works with the Tablicious classes I tested it on. (The help command does not work, but that is expected, because IMHO Octave's help function is kinda busted WRT third-party packages and code.)

Would you mind squashing your commits, and rewording the commit message to better describe what this change fixes? In the commits for this PR, I see three separate commits:

image

I'd rather those be a single commit, and instead of saying just "Update load_tablicious.m" etc, they described why the change was made, and what problem it was addressing. Like "Fix library initialization for Octave 7.x" or something like that.

Also, in the future, it'd be convenient if you had your changes for PRs on a branch in your fork that wasn't master. We can still do a master->master PR, but that gets messy in more-active repos, where there may be additional changes on the upstream master, or multiple PRs.

Do me a favor and clean this up git-wise and I'll merge it? Code here looks good; I'd just like to do the merge in a way that both keeps the Tablicious repo's git history tidy, and also preserves author credit for you.

apjanke commented 2 years ago

BTW, thanks for this contribution! This looks like the right approach, and it would have taken me some time to get there myself, and I'm really short on time at the moment, because Reasons.

gkourachanis commented 2 years ago

Code is looking good. I tested this under Octave 7.1.0 on macOS, and it loads up no problem, and the doc command works with the Tablicious classes I tested it on. (The help command does not work, but that is expected, because IMHO Octave's help function is kinda busted WRT third-party packages and code.)

Would you mind squashing your commits, and rewording the commit message to better describe what this change fixes? In the commits for this PR, I see three separate commits:

image

I'd rather those be a single commit, and instead of saying just "Update load_tablicious.m" etc, they described why the change was made, and what problem it was addressing. Like "Fix library initialization for Octave 7.x" or something like that.

Also, in the future, it'd be convenient if you had your changes for PRs on a branch in your fork that wasn't master. We can still do a master->master PR, but that gets messy in more-active repos, where there may be additional changes on the upstream master, or multiple PRs.

Do me a favor and clean this up git-wise and I'll merge it? Code here looks good; I'd just like to do the merge in a way that both keeps the Tablicious repo's git history tidy, and also preserves author credit for you.

Hey! Everything is fine now. BTW, I had initially commited those changes from GitHub's web interface and I couldn't find squashing option. Anyway, I cloned it locally and properly made those changes.

BTW, thanks for this contribution! This looks like the right approach, and it would have taken me some time to get there myself, and I'm really short on time at the moment, because Reasons.

No problem! I'm glad I could help :)

apjanke commented 2 years ago

Looks good now! I'll re-test this and hopefully get it merged tonight.

apjanke commented 1 year ago

Merged! Thanks for the contribution, and sorry for the slow response on my part. It's been... a week. For several weeks in a row.