Port of is_probably_readerable from mozilla

[zirkelc]

Port of isProbablyReaderable from Mozilla.

In the other PR, I have used BeautifulSoup and in this I'm using the LXML functions to select the nodes. For whatever reason, there seems to be a difference in the result when running it on the evaluation dataset. I already debugged it, but couldn't figure it out yet. The XPath selector selects a different set of elements than the CSS selector. Maybe you can spot a difference that I'm overlooking?

I didn't include any tests yet. The official function from Mozilla uses the following test cases to test for isProbablyReaderable=False (see attribute "readerable": false):

• https://github.dev/mozilla/readability/blob/main/test/test-pages/nytimes-1/expected-metadata.json
• https://github.dev/mozilla/readability/blob/main/test/test-pages/nytimes-1/expected-metadata.json
• https://github.dev/mozilla/readability/blob/main/test/test-pages/nytimes-1/expected-metadata.json
• https://github.dev/mozilla/readability/blob/main/test/test-pages/nytimes-1/expected-metadata.json

I can copy these files if you think it makes sense.

Please let me know if there is anything to be changed or added.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.94%. Comparing base (efe38bb) to head (5085429).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
+ Coverage   97.81%   97.94%   +0.12%     
==========================================
  Files          21       21              
  Lines        3437     3496      +59     
==========================================
+ Hits         3362     3424      +62     
+ Misses         75       72       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adbar]

Hi @zirkelc, thanks for the PR, the code looks good.
Yes, please add basic tests for the functions you added. At best in a new function in tests/unit_tests.py or in a new test script. The Mozilla tests would be fine, it could also be a HTML snippet if you can isolate the right parts, as you prefer.

It could be that the combination LXML+XPath works slightly differently than your original setting (BS4+CSS selector), could you please tell me where exactly the difference occurs?

[zirkelc]

Hi @adbar

thanks for the feedback! I ported the unit tests from mozilla and one test page . Let me know if I should add or change something.

Regarding the XPath vs CSS selector: I will debug it once for to see if I can isolate the discrepancy.

[adbar]

A test is failing because you added a file to the resource directory, could you please fix this?

assert 10 <= len(list(cli_utils.generate_filelist(RESOURCES_DIR))) <= 20 in cli_tests.py::test_input_type()

[zirkelc]

Sorry, I should have ran all tests. I increased the assertions to 21. Is it okay to put the file into resources or is another folder better suited?

[adbar]

It works, thanks. You should now add tests for the lines not covered by the HTML file (see coverage report in "Files changed" tab).

[zirkelc]

I added some more tests which should cover the lines. I also changed the XPath and added test for it

[adbar]

There is something wrong with the syntax, see flake error message.

[adbar]

I assume the problem is related to nested quotation marks, could you check again?

[adbar]

Thanks, now the tests pass. I entered a series of minor changes to implement, the PR can soon be merged.

[zirkelc]

Thanks, please take a look at the changes

[adbar]

LGTM.

[adbar]

Additional notes:

• The regular expressions used here are slightly different from the legacy ones at the top of the file, probably because they're newer? It would be nice to test if the old ones have to be replaced.
• The additional functions could be documented before a new version comes out.

[zirkelc]

Additional notes:

The regular expressions used here are slightly different from the legacy ones at the top of the file, probably because they're newer? It would be nice to test if the old ones have to be replaced.

Yes, the regex are newer with some additional classes:

https://github.com/mozilla/readability/blob/97db40ba035a2de5e42d1ac7437893cf0da31d76/Readability.js#L123-L126

However, as readability_lxml is based on two other ports of readability and therefore probably out of sync with readability in general, maybe a completely new port from the JS code would make sense in this case?

• The additional functions could be documented before a new version comes out.

Should I add something to the docs?

[adbar]

I can take care of the docs before the next release and you can improve on that later if you want.

As you say the readability_lxml module is out of sync but it's not completely different either so it's probably adaptable. If you wish to improve the current state you're very welcome to do so. Bit by bit (maybe easier) or with a complete redesign, as you prefer.

The current version achieves a fine balance between accuracy and precision, so at best changes would need to be tested on the Trafilatura's benchmark (and/or on more comprehensive benchmarks).