-
Notifications
You must be signed in to change notification settings - Fork 119
ignore any exception if errors='ignore' #100
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
=========================================
- Coverage 88.7% 87.3% -1.41%
=========================================
Files 11 11
Lines 434 457 +23
Branches 89 97 +8
=========================================
+ Hits 385 399 +14
- Misses 44 52 +8
- Partials 5 6 +1
Continue to review full report at Codecov.
|
lopuhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Kebniss , looks good 👍 left one small comment. Regarding testing, passing an empty string as html will trigger an error in the parser (document is empty), so I think we could use it for a test.
lopuhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @Kebniss ! Left a comment regarding more granular exception handling in unification.
lopuhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 left a few minor comments
|
codecov fails because some exceptions are not tested, however I cannot come up with an input that is extracted ok but then fails when uniforming data |
lopuhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kebniss looks good! I think it's expected that we can't test all exception code paths. Left one small comment, apart from that it's ready to merge 👍
Co-Authored-By: Kebniss <ludovica.gonella@gmail.com>
lopuhin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @Kebniss 👍
as explained in #99 errors='ignore' was not universal.
I wanted to add a test but I don't know how to test exceptions