Be more tolerant to invalid css. #175
Conversation
mgautierfr
force-pushed
the
css_error_tolerant
branch
from
d471fdf to
35c08b6
Compare
|
New version rewrite the css content using regex if tinycss2 fails to parse it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## warc2zim2 #175 +/- ##
=============================================
+ Coverage 86.57% 87.08% +0.50%
=============================================
Files 13 13
Lines 849 867 +18
Branches 147 149 +2
=============================================
+ Hits 735 755 +20
+ Misses 97 96 -1
+ Partials 17 16 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
See comments.
Please add more tests to have 100% test coverage on the patch : https://app.codecov.io/gh/openzim/warc2zim/pull/175?src=pr&el=tree ; no need to do all test cases on all code branches, but at least have one test case on each of them, especially since here you are not testing the inline situation which is the one we have in production.
I think that we need:
- logs when the normal CSS rewriting failed (a warning probably)
- exception handling for the fallback rewriter: maybe simply a clear log with original content, because this is probably a very rare situation (I don't see why regex would fail) but it would make debugging way simpler
- more fine-grained processing of CSS rules ; since we know that tinycss is permissive in terms of parsing, we could process / serialize CSS rules one by one and apply the fallback only on failing CSS rules (this could be done in a separate issue)
pyright automatically "promote" `bytes` type to `bytes|memoryview`.
So `str|bytes` is promoted to `str|bytes|memoryview` and the following
doesn't change the type of `bytes_or_text` variable as `memoryview` is not
covered.
```python
if instance(bytes_or_text, bytes):
bytes_or_text = bytes_or_text.decode()
```
By disabling bytes type promoting, all cases are covered and
`bytes_or_text` is "converted" to `str`.
mgautierfr
force-pushed
the
css_error_tolerant
branch
2 times, most recently
from
d6b1c89 to
88bce6f
Compare
There was a problem hiding this comment.
All conversations have been addressed properly, thank you (and great!), but AFAIK you didn't provided feedback on the last 3 recommendations of #175 (review)
|
Sorry, I forget about these comments
Indeed, will do
As you say, I don't see why regex would fail. If it is the case, it is more a bug in our code than a invalid input we have to handle somehow.
I have think about that, but not an easy task. We would have to reimplement tinycss2 serialize feature. And on top of that, it is not clear about what to do in case of parsing error. Even tinycss try to be smart and recover error but give up on parsing error (https://github.com/Kozea/tinycss2/blob/main/tinycss2/ast.py#L105-L115). |
OK, let's live with it for now.
Agreed! I've opened an issue to keep this information in mind: #176 |
When tinycss2 fails to parse, we use a regex rewriter to (trying to) rewrite url in the css. In case of invalid css tinycss2 doesn't not failed on parsing, but generate a ParserError token https://doc.courtbouillon.org/tinycss2/stable/api_reference.html?highlight=parser#tinycss2.ast.ParseError This is on serialization that exception occurs. We even try/except all transformation to be sure. Fix #155
benoit74
force-pushed
the
css_error_tolerant
branch
from
db8c7ac to
e9c1743
Compare
Return the original content if we cannot parse the css.
In case of invalid css tinycss2 doesn't not failed on parsing, but generate a ParserError token
https://doc.courtbouillon.org/tinycss2/stable/api_reference.html?highlight=parser#tinycss2.ast.ParseError
This is on serialization that exception occurs.
We even try/except all transformation to be sure.
Fix #155