Page MenuHomePhabricator

[python] Support PathLike filenames and directories
ClosedPublic

Authored by jstasiak on Nov 5 2018, 1:04 PM.

Details

Summary

Python 3.6 introduced a file system path protocol (PEP 519[1]). The standard library APIs accepting file system paths now accept path objects too. It could be useful to add this here as well for convenience.

[1] https://www.python.org/dev/peps/pep-0519

Diff Detail

Repository
rL LLVM

Event Timeline

jstasiak created this revision.Nov 5 2018, 1:04 PM
mgorny requested changes to this revision.Nov 10 2018, 12:19 AM

Also please remember to submit patches with -U9999, so that Phab has full context.

bindings/python/tests/cindex/test_cdb.py
42 ↗(On Diff #172641)

Please use conditional skipping instead. Maybe write a @skip_if_no_fspath decorator in util.py and use it for all tests. Skipping will have the advantage that instead of silently pretending we have less tests, we'd explicitly tell user why some of the tests aren't run.

bindings/python/tests/cindex/util.py
6 ↗(On Diff #172641)

Wouldn't it be better to use if HAS_FSPATH here? In normal circumstances, the pathlib import will be unnecessarily attempted (and it will succeed if pathlib package is installed), even though it will never be used inside tests.

This revision now requires changes to proceed.Nov 10 2018, 12:19 AM
jstasiak updated this revision to Diff 173496.Nov 10 2018, 1:06 AM

Tests are skipped using unittest skipping mechanism now and pathlib imported only when necessary.

jstasiak marked 2 inline comments as done.Nov 10 2018, 1:09 AM

Thanks for the feedback, you're right those were things worth improving, I updated the code.

mgorny accepted this revision.Nov 10 2018, 2:51 AM

LGTM, thanks!

bindings/python/clang/cindex.py
133 ↗(On Diff #173496)

Optionally: this is now a little bit nitpick but could you use a different variable name (e.g. commonly used elsewhere x)? This could be a little bit confusing with Python's string module.

This revision is now accepted and ready to land.Nov 10 2018, 2:51 AM
jstasiak updated this revision to Diff 173505.Nov 10 2018, 3:03 AM

That's fair, changed string to just x, should be obvious from the context what x is.

Thank you for the review. As I don't have commit access I'd like to ask you to commit this on my behalf.

This revision was automatically updated to reflect the committed changes.

Merged. I will get back to you if something explodes ;-).