Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added possibility to prune xPaths #414

Merged
merged 6 commits into from
Oct 4, 2023
Merged

added possibility to prune xPaths #414

merged 6 commits into from
Oct 4, 2023

Conversation

HeLehm
Copy link
Contributor

@HeLehm HeLehm commented Sep 1, 2023

Same as I did in #259 .

Also: I added a simple test.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #414 (de9c516) into master (0fbc1cb) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   96.82%   96.67%   -0.15%     
==========================================
  Files          22       22              
  Lines        3334     3338       +4     
==========================================
- Hits         3228     3227       -1     
- Misses        106      111       +5     
Files Coverage Δ
trafilatura/core.py 97.50% <100.00%> (-0.49%) ⬇️

... and 1 file with indirect coverage changes

@adbar
Copy link
Owner

adbar commented Sep 1, 2023

@HeLehm Thanks, the PR looks good, I'll hopefully merge it soon.

@HeLehm
Copy link
Contributor Author

HeLehm commented Sep 14, 2023

Hi @adbar, any updates on this? Can I do something to help?

@adbar
Copy link
Owner

adbar commented Sep 15, 2023

Hi @HeLehm, I'll be afk for a little while but I'll merge your PR in October at the latest. In the meantime, could you please add the following tests? Not strictly speaking a coverage problem, rather a logical way to cover everything:

  • You only test for str for prune_xpath, please also test with a list
  • In your test the whole document is stripped, just add a test to make sure the right amount of text is removed

The two items can be combined.

@adbar
Copy link
Owner

adbar commented Oct 2, 2023

Thanks! I don't quite understand why the coverage changes should be an issue and I plan to merge the PR as it is.

@adbar adbar merged commit 7dd7347 into adbar:master Oct 4, 2023
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants