From 55b26dfc04c9567e12ed11798ae361ca06219c2e Mon Sep 17 00:00:00 2001 From: Nick Medveditskov Date: Fri, 30 Mar 2018 22:13:11 -0700 Subject: [PATCH 1/2] `InvalidPackage` exception has 'msg', `package_path` and defines '__str__' Note that `parse_package_string` catches InvalidPackage exception and sets the filename; it is needed because the functions that called in `_parse_package_string` raise InvalidPackage exception without filename, e.g. `_get_node` --- src/catkin_pkg/package.py | 33 +++++++++++++++++---------------- src/catkin_pkg/python_setup.py | 2 +- test/test_package.py | 12 ++++++++++++ 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/src/catkin_pkg/package.py b/src/catkin_pkg/package.py index 9bb89261..04b00796 100644 --- a/src/catkin_pkg/package.py +++ b/src/catkin_pkg/package.py @@ -154,7 +154,7 @@ def get_build_type(self): return 'catkin' if len(build_type_exports) == 1: return build_type_exports[0] - raise InvalidPackage('Only one element is permitted.') + raise InvalidPackage('Only one element is permitted.', self.filename) def has_invalid_metapackage_dependencies(self): """ @@ -209,13 +209,6 @@ def validate(self, warnings=None): :param warnings: Print warnings if None or return them in the given list :raises InvalidPackage: in case validation fails """ - try: - self._validate(warnings) - except InvalidPackage as e: - e.args = ('Invalid package manifest "{0}": {1}'.format(self.filename, e),) - raise - - def _validate(self, warnings): errors = [] new_warnings = [] @@ -256,7 +249,7 @@ def _validate(self, warnings): try: maintainer.validate() except InvalidPackage as e: - errors.append(str(e)) + errors.append(e.msg) if not maintainer.email: errors.append('Maintainers must have an email address') @@ -270,7 +263,7 @@ def _validate(self, warnings): try: author.validate() except InvalidPackage as e: - errors.append(str(e)) + errors.append(e.msg) dep_types = { 'build': self.build_depends, @@ -309,7 +302,7 @@ def _validate(self, warnings): warnings.append(warning) if errors: - raise InvalidPackage('\n'.join(errors)) + raise InvalidPackage('\n'.join(errors), self.filename) class Dependency(object): @@ -421,7 +414,14 @@ def parse_package_for_distutils(path=None): class InvalidPackage(Exception): - pass + def __init__(self, msg, package_path=None): + self.msg = msg + self.package_path = package_path + Exception.__init__(self, self.msg) + + def __str__(self): + result = '' if not self.package_path else 'Error(s) in package \'%s\':\n' % self.package_path + return result + Exception.__str__(self) def package_exists_at(path): @@ -503,7 +503,8 @@ def parse_package_string(data, filename=None, warnings=None): try: return _parse_package_string(data, filename=filename, warnings=warnings) except InvalidPackage as e: - e.args = ('Invalid package manifest "{0}": {1}'.format(filename, e),) + if not e.package_path: + e.package_path = filename raise @@ -511,14 +512,14 @@ def _parse_package_string(data, filename=None, warnings=None): try: root = dom.parseString(data) except Exception as ex: - raise InvalidPackage('The manifest contains invalid XML:\n%s' % ex) + raise InvalidPackage('The manifest contains invalid XML:\n%s' % ex, filename) pkg = Package(filename) # verify unique root node nodes = _get_nodes(root, 'package') if len(nodes) != 1: - raise InvalidPackage('The manifest must contain a single "package" root tag') + raise InvalidPackage('The manifest must contain a single "package" root tag', filename) root = nodes[0] # format attribute @@ -671,7 +672,7 @@ def _parse_package_string(data, filename=None, warnings=None): errors.append('The "%s" tag must not contain the following children: %s' % (node.tagName, ', '.join([n.tagName for n in subnodes]))) if errors: - raise InvalidPackage('Error(s):%s' % (''.join(['\n- %s' % e for e in errors]))) + raise InvalidPackage('Error(s):%s' % (''.join(['\n- %s' % e for e in errors])), filename) pkg.validate(warnings=warnings) diff --git a/src/catkin_pkg/python_setup.py b/src/catkin_pkg/python_setup.py index 4832fa8a..8f8453f3 100644 --- a/src/catkin_pkg/python_setup.py +++ b/src/catkin_pkg/python_setup.py @@ -112,7 +112,7 @@ def generate_distutils_setup(package_xml_path=os.path.curdir, **kwargs): for k, v in kwargs.items(): if k in data: if v != data[k]: - raise InvalidPackage('The keyword argument "%s" does not match the information from package.xml: "%s" != "%s"' % (k, v, data[k])) + raise InvalidPackage('The keyword argument "%s" does not match the information from package.xml: "%s" != "%s"' % (k, v, data[k]), package_xml_path) else: data[k] = v diff --git a/test/test_package.py b/test/test_package.py index c637c70e..4a2ddbee 100644 --- a/test/test_package.py +++ b/test/test_package.py @@ -261,6 +261,18 @@ def test_validate_package(self): self.assertRaises(InvalidPackage, Package.validate, pack) dep_type.remove(depend) + def test_invalid_package_exception(self): + try: + raise InvalidPackage('foo') + except InvalidPackage as e: + self.assertEqual('foo', str(e)) + self.assertIsNone(e.package_path) + try: + raise InvalidPackage('foo', package_path='\\bar') + except InvalidPackage as e: + self.assertEqual('Error(s) in package \'\\bar\':\nfoo', str(e)) + self.assertEqual('\\bar', e.package_path) + def test_validate_person(self): auth1 = Person('foo') auth1.email = 'foo@bar.com' From 40ffdcdc4cf96caf0ed77c77099d1eb871a22364 Mon Sep 17 00:00:00 2001 From: Nick Medveditskov Date: Fri, 30 Mar 2018 22:17:42 -0700 Subject: [PATCH 2/2] `assertIsNone()` does not exists in Python 2.6, replace with `self.assertEqual(None, ...)`` --- test/test_package.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_package.py b/test/test_package.py index 4a2ddbee..ce45761d 100644 --- a/test/test_package.py +++ b/test/test_package.py @@ -266,7 +266,7 @@ def test_invalid_package_exception(self): raise InvalidPackage('foo') except InvalidPackage as e: self.assertEqual('foo', str(e)) - self.assertIsNone(e.package_path) + self.assertEqual(None, e.package_path) try: raise InvalidPackage('foo', package_path='\\bar') except InvalidPackage as e: