WIP: make lxml (and singledispatch) optional #161
Conversation
works with both lxml and xml.etree backends adds some missing things from built-in etree, such as the ability to use an OrderedDict for attributes, support for pretty_print argument to add indentation, etc.
one can always do `pytest -v`, or through `tox -- -v`
this is conventional among python packages to have an 'testing' extras_require so developers can bootstrap a testing environment easily with something like: pip install -e .[testing] The pytest-cov is a pluging to integrate coverage.py with pytest. pytest-randomly is to shuffle tests in a random order to check against inter-test dependencies
…decov.io and remove 3.5, we don't need to test that one any more now that there's 3.7
and skip_branch_with_pr, to avoid running CI twice both on the PR branch and on merged master.
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
=========================================
Coverage ? 88.73%
=========================================
Files ? 20
Lines ? 10157
Branches ? 1106
=========================================
Hits ? 9013
Misses ? 773
Partials ? 371
Continue to review full report at Codecov.
|
…thon.exe https://nedbatchelder.com/blog/201509/appveyor.html The old TOXPYTHON trick seems to be discouraged now with tox 3.1, which emits a warning when there is a conflict in the basepython settings for environments containing default factors (e.g. py27, etc.) tox-dev/tox#841
anthrotype
force-pushed
the
optional-lxml
branch
from
c96462f to
38a9b5f
Compare
anthrotype
force-pushed
the
optional-lxml
branch
from
38a9b5f to
3ca6320
Compare
anthrotype
force-pushed
the
optional-lxml
branch
from
7aeffbb to
0951e20
Compare
this had been broken for years, since commit 66e5ae0
|
@anthrotype strings are still saved as data in All of these string vs. data issues make me think that the tests have some glaring holes. What's your opinion? |
Yeah, that’s why I’m adding coverage support in this pr among other things. |
looks like defcon LayerSet.newLayer does not ensure the 'name' argument is a unicode string, so serializing the layercontents.plist with the new ufoLib.plistlib may lead to the layer name being encoded as a <data> element instead of a <string>. Thanks Miguel for noticing this.
|
42a3428 fixes |
|
Right.. will fix tomorrow |
hm, I don't think it's possible for the self.layerContents values (the directory name) to not be unicode, because the directory name is generated by the @miguelsousa did you actually manage to reproduce the directory value in layerContents being a bytes string instead of unicode string? |
|
btw @miguelsousa sorry I didn't make it in time for the pyup.io bot weekly update-all-the-repos.. Let's not rush this, and make sure we don't introduce any further regressions. |
… notation [ci skip]
|
ok i'm gonna merge and release 2.3.0. I don't think this deserves 3.0.0. We are not breaking the API, the lxml dependency is optional, though recommended for better speeds. |
|
super, thanks!! |
|
Thanks! Does this then close #140? |
yes. I patched mutatorMath locally, updated ufoLib to 42a3428, and ran
agree. No rush on our end. |
The reason I haven't tagged ufoLib 3.0 yet is because I was working on allowing to use the built-in
xml.etreemodule when lxml is not installed.The reason is, as much as I love to add third-party dependencies (:grin) and avoid to re-invent the wheel, I believe it makes sense to keep ufoLib working in a simple, pure-python setup, like it was before.
Especially since we are also planning to eventually merge ufoLib into fonttools, where third-party dependencies are even more difficult to add.
this PR adds a
ufoLib.etreemodule which exports the same API as the (built-in)xml.etree.ElementTreeorlxml.etree, and uses the latter whenever it is available otherwise falls back on the built-in module.In order to produce the same output (with control over the order of the attributes, etc.), I needed to subclass Element and ElementTree objects and basically adapt the serialization part of the built-in ElementTree library.
The current tests pass, which is good. Of course, it is a bit slower than lxml (havent' run benchmarks yet), but the idea is to have it as a reasonable default or fallback, and lxml being the opt-in extra for those who want faster output.
I would like to add some more tests before merging this.