New codemod: safe-lxml-parsing
#64
Conversation
Codecov Report
@@ Coverage Diff @@
## main #64 +/- ##
==========================================
+ Coverage 95.82% 95.86% +0.04%
==========================================
Files 45 46 +1
Lines 1676 1694 +18
==========================================
+ Hits 1606 1624 +18
Misses 70 70
|
clavedeluna
force-pushed
the
safe-lxml-parsing
branch
from
e39cd60 to
d833a92
Compare
| var = "hello" | ||
| """ | ||
| expexted_output = f"""from lxml.etree import {func} | ||
| import lxml |
There was a problem hiding this comment.
I will point out that this double import is not idea, but it does work and is valid code. Our other codemods (tempfile) also do this at this time.
|
|
||
| def on_result_found(self, original_node, updated_node): | ||
| self.remove_unused_import(original_node) | ||
| self.add_needed_import("lxml") |
There was a problem hiding this comment.
I don't think this is sufficient:
>>> import lxml
>>> lxml.etree.XMLParser()
AttributeError Traceback (most recent call last)
Cell In[2], line 1
----> 1 lxml.etree.XMLParser
AttributeError: module 'lxml' has no attribute 'etree'I think it needs to be import lxml.etree.
Not trying to pick on your PR in particular (because it's very nice overall) but imo this is one fairly strong argument for functional integration tests.
There was a problem hiding this comment.
woah, really good catch here. In fact it's so correct there are no examples of import lxml ... lxml.etree online! I will update the tests accordingly.
There was a problem hiding this comment.
I actually had to change BOTH lxml codemods, great find! Glad I waited for the docs
There was a problem hiding this comment.
This really hammers in the need for functional-level tests for me. Any of us could have (and probably have) made similar mistakes, but it's really hard to know whether we're generating correct code unless it actually executes.
I think the right way to think about this is: what kind of test would have alerted you to this problem immediately? In this particular case, I think that just importing the output of the existing integration test would have caught it, so maybe that's a good starting point for functional integration tests.
There was a problem hiding this comment.
I'm fully convinced! I will follow your lead in prioritizing that work and happy to take it on.
clavedeluna
force-pushed
the
safe-lxml-parsing
branch
from
d833a92 to
6cc4453
Compare
clavedeluna
force-pushed
the
safe-lxml-parsing
branch
from
6cc4453 to
91c4f4b
Compare
clavedeluna
force-pushed
the
safe-lxml-parsing
branch
from
1b56f1b to
62d4046
Compare
| @@ -18,7 +18,7 @@ jobs: | |||
| linting: | |||
| name: Pylint & Black Checks | |||
| runs-on: ubuntu-20.04 | |||
| timeout-minutes: 3 | |||
| timeout-minutes: 5 | |||
Overview
A new codemod that will check calls to
lxml.etree.parseandlxml.etree.fromstringDescription
safe-lxml-parser-defaultscodemod will kick in (if enabled)