[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny added a comment. Merged. I will get back to you if something explodes ;-). Repository: rL LLVM https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346586: [python] Support PathLike filenames and directories
(authored by mgorny, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54120?vs=173505=173508#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54120
Files:
cfe/trunk/bindings/python/clang/cindex.py
cfe/trunk/bindings/python/tests/cindex/test_cdb.py
cfe/trunk/bindings/python/tests/cindex/test_code_completion.py
cfe/trunk/bindings/python/tests/cindex/test_translation_unit.py
cfe/trunk/bindings/python/tests/cindex/util.py
Index: cfe/trunk/bindings/python/clang/cindex.py
===
--- cfe/trunk/bindings/python/clang/cindex.py
+++ cfe/trunk/bindings/python/clang/cindex.py
@@ -67,6 +67,7 @@
import clang.enumerations
+import os
import sys
if sys.version_info[0] == 3:
# Python 3 strings are unicode, translate them to/from utf8 for C-interop.
@@ -123,6 +124,14 @@
def b(x):
return x
+# We only support PathLike objects on Python version with os.fspath present
+# to be consistent with the Python standard library. On older Python versions
+# we only support strings and we have dummy fspath to just pass them through.
+try:
+fspath = os.fspath
+except AttributeError:
+def fspath(x):
+return x
# ctypes doesn't implicitly convert c_void_p to the appropriate wrapper
# object. This is a problem, because it means that from_parameter will see an
@@ -2752,11 +2761,11 @@
etc. e.g. ["-Wall", "-I/path/to/include"].
In-memory file content can be provided via unsaved_files. This is an
-iterable of 2-tuples. The first element is the str filename. The
-second element defines the content. Content can be provided as str
-source code or as file objects (anything with a read() method). If
-a file object is being used, content will be read until EOF and the
-read cursor will not be reset to its original position.
+iterable of 2-tuples. The first element is the filename (str or
+PathLike). The second element defines the content. Content can be
+provided as str source code or as file objects (anything with a read()
+method). If a file object is being used, content will be read until EOF
+and the read cursor will not be reset to its original position.
options is a bitwise or of TranslationUnit.PARSE_XXX flags which will
control parsing behavior.
@@ -2801,11 +2810,13 @@
if hasattr(contents, "read"):
contents = contents.read()
-unsaved_array[i].name = b(name)
+unsaved_array[i].name = b(fspath(name))
unsaved_array[i].contents = b(contents)
unsaved_array[i].length = len(contents)
-ptr = conf.lib.clang_parseTranslationUnit(index, filename, args_array,
+ptr = conf.lib.clang_parseTranslationUnit(index,
+fspath(filename) if filename is not None else None,
+args_array,
len(args), unsaved_array,
len(unsaved_files), options)
@@ -2826,11 +2837,13 @@
index is optional and is the Index instance to use. If not provided,
a default Index will be created.
+
+filename can be str or PathLike.
"""
if index is None:
index = Index.create()
-ptr = conf.lib.clang_createTranslationUnit(index, filename)
+ptr = conf.lib.clang_createTranslationUnit(index, fspath(filename))
if not ptr:
raise TranslationUnitLoadError(filename)
@@ -2983,7 +2996,7 @@
print(value)
if not isinstance(value, str):
raise TypeError('Unexpected unsaved file contents.')
-unsaved_files_array[i].name = name
+unsaved_files_array[i].name = fspath(name)
unsaved_files_array[i].contents = value
unsaved_files_array[i].length = len(value)
ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
@@ -3002,10 +3015,10 @@
case, the reason(s) why should be available via
TranslationUnit.diagnostics().
-filename -- The path to save the translation unit to.
+filename -- The path to save the translation unit to (str or PathLike).
"""
options = conf.lib.clang_defaultSaveOptions(self)
-result = int(conf.lib.clang_saveTranslationUnit(self, filename,
+result = int(conf.lib.clang_saveTranslationUnit(self, fspath(filename),
options))
if result != 0:
raise TranslationUnitSaveError(result,
@@ -3047,10 +3060,10 @@
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak updated this revision to Diff 173505.
jstasiak added a comment.
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.
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindings/python/tests/cindex/test_code_completion.py
bindings/python/tests/cindex/test_translation_unit.py
bindings/python/tests/cindex/util.py
Index: bindings/python/tests/cindex/util.py
===
--- bindings/python/tests/cindex/util.py
+++ bindings/python/tests/cindex/util.py
@@ -1,5 +1,15 @@
# This file provides common utility functions for the test suite.
+import os
+HAS_FSPATH = hasattr(os, 'fspath')
+
+if HAS_FSPATH:
+from pathlib import Path as str_to_path
+else:
+str_to_path = None
+
+import unittest
+
from clang.cindex import Cursor
from clang.cindex import TranslationUnit
@@ -68,8 +78,13 @@
return cursors
+skip_if_no_fspath = unittest.skipUnless(HAS_FSPATH,
+"Requires file system path protocol / Python 3.6+")
+
__all__ = [
'get_cursor',
'get_cursors',
'get_tu',
+'skip_if_no_fspath',
+'str_to_path',
]
Index: bindings/python/tests/cindex/test_translation_unit.py
===
--- bindings/python/tests/cindex/test_translation_unit.py
+++ bindings/python/tests/cindex/test_translation_unit.py
@@ -20,6 +20,8 @@
from clang.cindex import TranslationUnit
from .util import get_cursor
from .util import get_tu
+from .util import skip_if_no_fspath
+from .util import str_to_path
kInputsDir = os.path.join(os.path.dirname(__file__), 'INPUTS')
@@ -36,6 +38,17 @@
yield t.name
+@contextmanager
+def save_tu_pathlike(tu):
+"""Convenience API to save a TranslationUnit to a file.
+
+Returns the filename it was saved to.
+"""
+with tempfile.NamedTemporaryFile() as t:
+tu.save(str_to_path(t.name))
+yield t.name
+
+
class TestTranslationUnit(unittest.TestCase):
def test_spelling(self):
path = os.path.join(kInputsDir, 'hello.cpp')
@@ -89,6 +102,22 @@
spellings = [c.spelling for c in tu.cursor.get_children()]
self.assertEqual(spellings[-1], 'x')
+@skip_if_no_fspath
+def test_from_source_accepts_pathlike(self):
+tu = TranslationUnit.from_source(str_to_path('fake.c'), ['-Iincludes'], unsaved_files = [
+(str_to_path('fake.c'), """
+#include "fake.h"
+int x;
+int SOME_DEFINE;
+"""),
+(str_to_path('includes/fake.h'), """
+#define SOME_DEFINE y
+""")
+])
+spellings = [c.spelling for c in tu.cursor.get_children()]
+self.assertEqual(spellings[-2], 'x')
+self.assertEqual(spellings[-1], 'y')
+
def assert_normpaths_equal(self, path1, path2):
""" Compares two paths for equality after normalizing them with
os.path.normpath
@@ -135,6 +164,16 @@
self.assertTrue(os.path.exists(path))
self.assertGreater(os.path.getsize(path), 0)
+@skip_if_no_fspath
+def test_save_pathlike(self):
+"""Ensure TranslationUnit.save() works with PathLike filename."""
+
+tu = get_tu('int foo();')
+
+with save_tu_pathlike(tu) as path:
+self.assertTrue(os.path.exists(path))
+self.assertGreater(os.path.getsize(path), 0)
+
def test_save_translation_errors(self):
"""Ensure that saving to an invalid directory raises."""
@@ -167,6 +206,22 @@
# Just in case there is an open file descriptor somewhere.
del tu2
+@skip_if_no_fspath
+def test_load_pathlike(self):
+"""Ensure TranslationUnits can be constructed from saved files -
+PathLike variant."""
+tu = get_tu('int foo();')
+self.assertEqual(len(tu.diagnostics), 0)
+with save_tu(tu) as path:
+tu2 = TranslationUnit.from_ast_file(filename=str_to_path(path))
+self.assertEqual(len(tu2.diagnostics), 0)
+
+foo = get_cursor(tu2, 'foo')
+self.assertIsNotNone(foo)
+
+# Just in case there is an open file descriptor somewhere.
+del tu2
+
def test_index_parse(self):
path = os.path.join(kInputsDir, 'hello.cpp')
index = Index.create()
@@ -185,6 +240,19 @@
with self.assertRaises(Exception):
f = tu.get_file('foobar.cpp')
+@skip_if_no_fspath
+def test_get_file_pathlike(self):
+"""Ensure tu.get_file() works appropriately with PathLike filenames."""
+
+tu = get_tu('int foo();')
+
+f = tu.get_file(str_to_path('t.c'))
+self.assertIsInstance(f, File)
+self.assertEqual(f.name, 't.c')
+
+with
[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: bindings/python/clang/cindex.py:133 +except AttributeError: +def fspath(string): +return string 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. https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak marked 2 inline comments as done. jstasiak added a comment. Thanks for the feedback, you're right those were things worth improving, I updated the code. https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak updated this revision to Diff 173496.
jstasiak added a comment.
Tests are skipped using unittest skipping mechanism now and pathlib imported
only when necessary.
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindings/python/tests/cindex/test_code_completion.py
bindings/python/tests/cindex/test_translation_unit.py
bindings/python/tests/cindex/util.py
Index: bindings/python/tests/cindex/util.py
===
--- bindings/python/tests/cindex/util.py
+++ bindings/python/tests/cindex/util.py
@@ -1,5 +1,15 @@
# This file provides common utility functions for the test suite.
+import os
+HAS_FSPATH = hasattr(os, 'fspath')
+
+if HAS_FSPATH:
+from pathlib import Path as str_to_path
+else:
+str_to_path = None
+
+import unittest
+
from clang.cindex import Cursor
from clang.cindex import TranslationUnit
@@ -68,8 +78,13 @@
return cursors
+skip_if_no_fspath = unittest.skipUnless(HAS_FSPATH,
+"Requires file system path protocol / Python 3.6+")
+
__all__ = [
'get_cursor',
'get_cursors',
'get_tu',
+'skip_if_no_fspath',
+'str_to_path',
]
Index: bindings/python/tests/cindex/test_translation_unit.py
===
--- bindings/python/tests/cindex/test_translation_unit.py
+++ bindings/python/tests/cindex/test_translation_unit.py
@@ -20,6 +20,8 @@
from clang.cindex import TranslationUnit
from .util import get_cursor
from .util import get_tu
+from .util import skip_if_no_fspath
+from .util import str_to_path
kInputsDir = os.path.join(os.path.dirname(__file__), 'INPUTS')
@@ -36,6 +38,17 @@
yield t.name
+@contextmanager
+def save_tu_pathlike(tu):
+"""Convenience API to save a TranslationUnit to a file.
+
+Returns the filename it was saved to.
+"""
+with tempfile.NamedTemporaryFile() as t:
+tu.save(str_to_path(t.name))
+yield t.name
+
+
class TestTranslationUnit(unittest.TestCase):
def test_spelling(self):
path = os.path.join(kInputsDir, 'hello.cpp')
@@ -89,6 +102,22 @@
spellings = [c.spelling for c in tu.cursor.get_children()]
self.assertEqual(spellings[-1], 'x')
+@skip_if_no_fspath
+def test_from_source_accepts_pathlike(self):
+tu = TranslationUnit.from_source(str_to_path('fake.c'), ['-Iincludes'], unsaved_files = [
+(str_to_path('fake.c'), """
+#include "fake.h"
+int x;
+int SOME_DEFINE;
+"""),
+(str_to_path('includes/fake.h'), """
+#define SOME_DEFINE y
+""")
+])
+spellings = [c.spelling for c in tu.cursor.get_children()]
+self.assertEqual(spellings[-2], 'x')
+self.assertEqual(spellings[-1], 'y')
+
def assert_normpaths_equal(self, path1, path2):
""" Compares two paths for equality after normalizing them with
os.path.normpath
@@ -135,6 +164,16 @@
self.assertTrue(os.path.exists(path))
self.assertGreater(os.path.getsize(path), 0)
+@skip_if_no_fspath
+def test_save_pathlike(self):
+"""Ensure TranslationUnit.save() works with PathLike filename."""
+
+tu = get_tu('int foo();')
+
+with save_tu_pathlike(tu) as path:
+self.assertTrue(os.path.exists(path))
+self.assertGreater(os.path.getsize(path), 0)
+
def test_save_translation_errors(self):
"""Ensure that saving to an invalid directory raises."""
@@ -167,6 +206,22 @@
# Just in case there is an open file descriptor somewhere.
del tu2
+@skip_if_no_fspath
+def test_load_pathlike(self):
+"""Ensure TranslationUnits can be constructed from saved files -
+PathLike variant."""
+tu = get_tu('int foo();')
+self.assertEqual(len(tu.diagnostics), 0)
+with save_tu(tu) as path:
+tu2 = TranslationUnit.from_ast_file(filename=str_to_path(path))
+self.assertEqual(len(tu2.diagnostics), 0)
+
+foo = get_cursor(tu2, 'foo')
+self.assertIsNotNone(foo)
+
+# Just in case there is an open file descriptor somewhere.
+del tu2
+
def test_index_parse(self):
path = os.path.join(kInputsDir, 'hello.cpp')
index = Index.create()
@@ -185,6 +240,19 @@
with self.assertRaises(Exception):
f = tu.get_file('foobar.cpp')
+@skip_if_no_fspath
+def test_get_file_pathlike(self):
+"""Ensure tu.get_file() works appropriately with PathLike filenames."""
+
+tu = get_tu('int foo();')
+
+f = tu.get_file(str_to_path('t.c'))
+self.assertIsInstance(f, File)
+self.assertEqual(f.name, 't.c')
+
+with self.assertRaises(Exception):
+f = tu.get_file(str_to_path('foobar.cpp'))
+
def
[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny requested changes to this revision. mgorny added a comment. This revision now requires changes to proceed. Also please remember to submit patches with `-U`, so that Phab has full context. Comment at: bindings/python/tests/cindex/test_cdb.py:42 +if HAS_FSPATH: +def test_lookup_succeed_pathlike(self): 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. Comment at: bindings/python/tests/cindex/util.py:6 + +try: +import pathlib 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. Repository: rC Clang https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak created this revision.
jstasiak added reviewers: mgorny, jbcoe.
Herald added a subscriber: arphaman.
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
Repository:
rC Clang
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindings/python/tests/cindex/test_code_completion.py
bindings/python/tests/cindex/test_translation_unit.py
bindings/python/tests/cindex/util.py
Index: bindings/python/tests/cindex/util.py
===
--- bindings/python/tests/cindex/util.py
+++ bindings/python/tests/cindex/util.py
@@ -1,5 +1,15 @@
# This file provides common utility functions for the test suite.
+import os
+HAS_FSPATH = hasattr(os, 'fspath')
+
+try:
+import pathlib
+except ImportError:
+str_to_path = None
+else:
+str_to_path = pathlib.Path
+
from clang.cindex import Cursor
from clang.cindex import TranslationUnit
@@ -72,4 +82,6 @@
'get_cursor',
'get_cursors',
'get_tu',
+'HAS_FSPATH',
+'str_to_path',
]
Index: bindings/python/tests/cindex/test_translation_unit.py
===
--- bindings/python/tests/cindex/test_translation_unit.py
+++ bindings/python/tests/cindex/test_translation_unit.py
@@ -20,6 +20,8 @@
from clang.cindex import TranslationUnit
from .util import get_cursor
from .util import get_tu
+from .util import HAS_FSPATH
+from .util import str_to_path
kInputsDir = os.path.join(os.path.dirname(__file__), 'INPUTS')
@@ -36,6 +38,17 @@
yield t.name
+@contextmanager
+def save_tu_pathlike(tu):
+"""Convenience API to save a TranslationUnit to a file.
+
+Returns the filename it was saved to.
+"""
+with tempfile.NamedTemporaryFile() as t:
+tu.save(str_to_path(t.name))
+yield t.name
+
+
class TestTranslationUnit(unittest.TestCase):
def test_spelling(self):
path = os.path.join(kInputsDir, 'hello.cpp')
@@ -89,6 +102,22 @@
spellings = [c.spelling for c in tu.cursor.get_children()]
self.assertEqual(spellings[-1], 'x')
+if HAS_FSPATH:
+def test_from_source_accepts_pathlike(self):
+tu = TranslationUnit.from_source(str_to_path('fake.c'), ['-Iincludes'], unsaved_files = [
+(str_to_path('fake.c'), """
+#include "fake.h"
+int x;
+int SOME_DEFINE;
+"""),
+(str_to_path('includes/fake.h'), """
+#define SOME_DEFINE y
+""")
+])
+spellings = [c.spelling for c in tu.cursor.get_children()]
+self.assertEqual(spellings[-2], 'x')
+self.assertEqual(spellings[-1], 'y')
+
def assert_normpaths_equal(self, path1, path2):
""" Compares two paths for equality after normalizing them with
os.path.normpath
@@ -135,6 +164,16 @@
self.assertTrue(os.path.exists(path))
self.assertGreater(os.path.getsize(path), 0)
+if HAS_FSPATH:
+def test_save_pathlike(self):
+"""Ensure TranslationUnit.save() works with PathLike filename."""
+
+tu = get_tu('int foo();')
+
+with save_tu_pathlike(tu) as path:
+self.assertTrue(os.path.exists(path))
+self.assertGreater(os.path.getsize(path), 0)
+
def test_save_translation_errors(self):
"""Ensure that saving to an invalid directory raises."""
@@ -167,6 +206,17 @@
# Just in case there is an open file descriptor somewhere.
del tu2
+# We can also pass the filename as Path-like object
+if HAS_FSPATH:
+tu2 = TranslationUnit.from_ast_file(filename=str_to_path(path))
+self.assertEqual(len(tu2.diagnostics), 0)
+
+foo = get_cursor(tu2, 'foo')
+self.assertIsNotNone(foo)
+
+# Just in case there is an open file descriptor somewhere.
+del tu2
+
def test_index_parse(self):
path = os.path.join(kInputsDir, 'hello.cpp')
index = Index.create()
@@ -185,6 +235,19 @@
with self.assertRaises(Exception):
f = tu.get_file('foobar.cpp')
+if HAS_FSPATH:
+def test_get_file_pathlike(self):
+"""Ensure tu.get_file() works appropriately with PathLike filenames."""
+
+tu = get_tu('int foo();')
+
+f = tu.get_file(str_to_path('t.c'))
+self.assertIsInstance(f, File)
+self.assertEqual(f.name, 't.c')
+
+with self.assertRaises(Exception):
+f = tu.get_file(str_to_path('foobar.cpp'))
+
def test_get_source_location(self):
"""Ensure
