maugenst / tabletojson

An npm module for node.js to convert HTML tables to JSON objects
https://www.npmjs.com/package/tabletojson
MIT License
138 stars 38 forks source link

Testing and coverage annotation prepared #27

Closed maugenst closed 6 years ago

maugenst commented 6 years ago

Hi Iain.

I worked on enabling testing and also added batches to be shown at the top of the README.md.

Currently npm run test would fail since the used travis-ci.org and coveralls.io are not added as trusted apps in github (travis-ci.org) or have not added .coveralls.yml (coveralls.io) what just you as the owner can do.

If you find it helpful please have a look into my forked repo. And if you think this could be the way to go please get back to me and I can help on adding the needed files...

CU, Marius

iaincollins commented 6 years ago

Oh wow, that's awesome!

I haven't finished going though everything yet, but having both tests and test coverage checking would be wonderful.

I'm familiar with Travis for CI and Istanbul for checking test coverage, but I haven't used Coveralls before. It looks like the tests pass but the coverage checking seems to fail locally (from a quick Google I gather it looks like it's designed to run inside Travis CI and/or against code in a repo online.).

$ npm test

> tabletojson@0.7.1 test /Users/iain/Development/tabletojson
> npm run mocha && npm run coverage

> tabletojson@0.7.1 mocha /Users/iain/Development/tabletojson
> node_modules/nyc/bin/nyc.js --reporter=text node_modules/mocha/bin/mocha

  TableToJSON Local
    ✓ Directly local html content: Table with header
    ✓ Directly passing html content: Table without header

  TableToJSON Remote
    ✓ Get table from Wikipedia using callBack function (683ms)
    ✓ Get table from Wikipedia using promise (461ms)
    ✓ Get table from timeanddate.com and use first row as heading (313ms)

  5 passing (2s)

----------------|----------|----------|----------|----------|-------------------|
File            |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------|----------|----------|----------|----------|-------------------|
All files       |    84.38 |    67.65 |      100 |    83.61 |                   |
 tabletojson.js |    84.38 |    67.65 |      100 |    83.61 |... 44,147,148,164 |
----------------|----------|----------|----------|----------|-------------------|

> tabletojson@0.7.1 coverage /Users/iain/Development/tabletojson
> node_modules/nyc/bin/nyc.js report --reporter=text-lcov node_modules/mocha/bin/mocha |node_modules/coveralls/bin/coveralls.js

/Users/iain/Development/tabletojson/node_modules/coveralls/bin/coveralls.js:18
        throw err;
        ^
Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

I've added you as collaborator if you'd like to push to master.

As this repo isn't in an organisation, it doesn't look like I can add you as an admin who can manage the settings for the repo.

I'm happy to either add this repo to an organisation to do that, to set up Travis for the Repo, or - and this might make the most sense if you are willing - to transfer ownership of the repo to you on GitHub and NPM, and for me to just remain on as a collaborator, with you as the owner going foward?

This has been a low maintenance project but I've got slightly more projects than I am managing to look after well and has been neglected by me as no-one has been complaining about it, and it might be good for it to have a better home where someone does a better job of tending to it? :-)

Regardless, I'm happy to continue however you like. I think this is a great way to go.

maugenst commented 6 years ago

Hi Iain. Thanks that you like the changes :-). I'm more than appreciated to help wherever I can and since I need this lib I was rather going to wait for changes or to help myself out :-).

So basically the current changes were mostly infrastructure related. The complete core and research was done by you. At least for this I would rather like to leave the project on your side (to take the credits ;-)). I also think that with having the tests, coverage and all the basic stuff set up there are not so much administrative tasks left.

To sum it up, I'm totally ok with leaving the ownership on your side, but if you think that I can speed up the game we can transfer it ;-) ... nonetheless I cannot decide it. You are the boss :-) ...

Looking forward to work with you! CU, Marius

maugenst commented 6 years ago

Hi Iain. So, if it is ok for you I would suggest to take it over. Just in case of you are too busy... Best regards, Marius

iaincollins commented 6 years ago

Hi Marius,

I think that would be a good idea if you are willing, as I am spread a little thin across a few projects.

Questions:

  1. Is this your account on NPM (so I can add you to the project there)? https://www.npmjs.com/~maugenst
  2. Should I transfer the repo to your GitHub account so you can do things like set up CI directly?

Thanks!

maugenst commented 6 years ago

Hi Iain. Yes, this is me ... maugenst at npmjs.com. I accidentally added an other name once, but the two packages aren't used anymore. And for consistence I would rather go for maugenst.

If you can transfer the ownership to my github account it would be great. So I would be able do the necessary changes...

Thanks and best regards, Marius

iaincollins commented 6 years ago

Oh excellent!

I've added you on NPM JS, though for GitHub it looks like you will need to rename your existing fork of tabletojson (as it gives the error maugenst/tabletojson already exists and the GitHub docs say you need to not have any existing repos or forks with the same name when transferring).

The changes you have done have made it much nicer and easier to maintain, thank you for everything you have done / are doing to make it better!

maugenst commented 6 years ago

Hi Iain ... I recently renamed the maugenst/tabletojson to tabletojsonNew. I hope this helps. Thanks for the kudos :-) ... CU, Marius

iaincollins commented 6 years ago

Huh, I figured that would work but it seems GitHub now says:

maugenst already has a repository in the iaincollins/tabletojson network

(Boo.)

If you want to merge this PR in for now (force push is fine as it's just the shrinkwrap that conflicts) and delete your fork I'll try transfer ownership again. :-)

maugenst commented 6 years ago

Hi Iain ... I'm currently on vacation. Sorry for not getting back to you in time. I resolved the shrinkwrap issue and merged it. I will also delete my fork right now. Thanks and CU, Marius

ps. I hope that I will be able to get back to you in the next few days ... I'll be back home at June 4th.

iaincollins commented 6 years ago

You should have the option to accept the transfer now.

Have a great vacation! :-)