Improve the check for Node

[GjjvdBurg]

This PR makes the check for Node more robust. It handles the scenario where a user installs ReadabiliPy, then installs Node, but doesn't reinstall ReadabiliPy. In that scenario the old have_node function would incorrectly return True.

[coveralls]

Coverage decreased (-0.4%) to 93.349% when pulling d22666e on GjjvdBurg:bugfix/have_node into 5543272 on alan-turing-institute:master.

[coveralls]

Coverage increased (+0.5%) to 94.266% when pulling 30d9156 on GjjvdBurg:bugfix/have_node into 5543272 on alan-turing-institute:master.

[martintoreilly]

@GjjvdBurg Sorry, I missed this in the autumn. I've just been looking into issue #92 and it now feels weird to me that we maintain (or more accurately don't maintain) a >2 year old local copy of Readability.js rather than just installing the latest version from Mozilla during the ReadabilPy install. Can you remember (or guess) why we decided to do this? I can only think that it maybe it's because of either / both of (i) we wanted to use a particular version of Readability.js (I can't think why, but maybe for reproducibility) and / or (ii) we don't want to actually install Readability.js on the user's computer.

What are your thoughts on dropping our outdated local copy and just installing the latest Readability.js from Mozilla during the ReadabiliPy install?

[martintoreilly]

I'll review this PR once I get the Javascript used in this package working on my machine. For some reason, I'm getting a missing package error from node for jsdom, despite it being required in our local javascript package. Even installing jsdom globally won't fix it, so I'm not sure what's going on.

[martintoreilly]

Your PR has already been helpful 🎉 . I notice that I don't have a node_modules dir in my javascript directory in my local git repo for ReadabiliPy, which a comment in your PR says means that ReadabiliPy was installed without node support.

However, I definitely had node installed prior to installing ReadabiliPy. First time round, I installed it with pip install -e . from my local git repo. I think I this may have installed in in a pyenv virtual environment (the VS code python window says pyenv shell 3.8.6).

I subsequently installed it using pip install readabilipy from the commandline and ran it using my default python (Python 3.8.6 (default, Oct 27 2020, 21:08:15)) and still no joy. Any thoughts?

[GjjvdBurg]

Hi @martintoreilly, thanks for checking up on this PR. It's likely you missed it initially because I marked it as draft PR. I didn't have a unit test for the added functionality at the time, and didn't get around to adding one because it would involve significant patching/mocking.

My contribution to this package is small as I only added the ability to install the node dependencies when installing the Python package. I can't tell you therefore why the version of Readability.js is fixed, but @jemrobinson might be able to do so. It does sound reasonable to also pull in Readability.js upon install, but I suspect that requires some changes to ExtractArticle.js.

To debug your installation issue, can you check what happens if you clone the repo and install using:

$ python setup.py -vv install --user

(either in a virtualenv or globally, it should either print out a warning or show node installation).

[jemrobinson]

I think that the version of Readability.js was fixed before my time. I mainly worked on the Python code which approximated the parts of Readability.js that we needed - I rarely used the JS wrapper functionality. I don't see a problem with updating to use the latest-available version unless the API has changed?

[GjjvdBurg]

@martintoreilly I finally added unit tests for the node check, and marked it as ready for review. Did you ever solve the installation issue you had?

I suggest we create a separate issue to remove the included version of Readability.js and pull it from npm instead (update: that's now in #95)

[jemrobinson]

LGTM