lightswitch05 / table-to-json

Serializes HTML tables into JSON objects.
http://lightswitch05.github.io/table-to-json/
MIT License
756 stars 172 forks source link

ignoreEmptyRows doesn't work in combination with ignoreColumns #53

Closed cn-tools closed 4 years ago

cn-tools commented 4 years ago

First: THX for this script!

in this jsfiddle you can see my test: https://jsfiddle.net/cntools/0cen5wu3/

the last row is empty, because the last column will be ignored. but in the json string the last row is available.

lightswitch05 commented 4 years ago

Hello @cn-tools - this is 100% a bug. What is happening is that the ignoreEmptyRows option does not consider the ignoreColumns option, and so it is not considering the row to be empty.

Full disclosure, I have not made a functional change to this tool since 2017 and I'm not sure I'm going to start back over this bug. I am glad so many people have found it useful, but I no longer use jQuery (moving on to tools like angular/react) - and so I do not have the motivation to use my limited free time working on something that is no longer of use to me personally and has not aged well technically speaking.

Depending on how critical you view this bug to be, you might want to have a look at the branch rewrite. As the name suggests, its a rewrite of this code base. From a quick glance, it appears that the rewrite code does not have this same bug. The logic to check if a row is empty or not is the same logic for creating the JSON output. Unfortunately, I never finished the rewrite branch - I believe it is missing the textExtractor and extractor options. I really wish I had been able to finish the rewrite branch when it was still fresh and I had more free time. It is significantly better code-wise - but unfinished as far as features go.

Of course I'll leave this bug ticket open in-case someone else cares to fix it. I'm happy to answer questions and merge any reasonable pull requests. I just don't have the time to fix it myself.

lightswitch05 commented 4 years ago

Also, THANK YOU for the awesome and very easy to understand jsfiddle! It made it a lot easier for me to verify this bug 👍 🥇

lightswitch05 commented 4 years ago

Fixed in release 0.13.1 - thanks @cn-tools!