New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture error location #244

Merged
merged 7 commits into from Apr 25, 2017

Conversation

Projects
None yet
2 participants
@bkline
Contributor

bkline commented Apr 24, 2017

Add path property to _LogEntry object.

This patch gets the XPath value from libxml2 for the node in which an error was detected (if available), and exposes that value as a property of the _LogEntry object, enabling user interface code to navigate the end user to the location of each error. Code which assembles xmlError structs outside of libxml2 is modified to initialize the node member of that struct. A typo in the assignment to the cached value for the _LogEntry's message property is also fixed.

Show outdated Hide outdated src/lxml/tests/test_xmlschema.py
@@ -80,7 +80,7 @@ def test_xmlschema_error_log_path(self):
schema.validate(tree)
tree_path = tree.getpath(tree.findall('b')[1])
error_path = schema.error_log[0].path
self.assertTrue(tree_path == error_path)
self.assertTrue(error_path is None or tree_path == error_path)

This comment has been minimized.

@scoder

scoder Apr 24, 2017

Member

Interesting test: the code might do what we think it does, but if it doesn't, that's fine, too. ;)
Could you explain why this was necessary?

@scoder

scoder Apr 24, 2017

Member

Interesting test: the code might do what we think it does, but if it doesn't, that's fine, too. ;)
Could you explain why this was necessary?

This comment has been minimized.

@bkline

bkline Apr 24, 2017

Contributor

Actually, looking back at http://www.xmlsoft.org/html/libxml-xmlerror.html#xmlError I think that was a mistake. I had read the comment earlier about the possibility that the parser context not be available ("the parser context if available"), and I mis-remembered that as applying to the xmlNode pointer as well, but looking back I see that there is no such caveat. We do know that there are xmlError structs floating around which have a NULL node pointer (we're creating some ourselves), but a good case could be made that the test should just assume that the pointer will always be non-NULL for schema validation errors. As you can see, that's how I originally wrote the test. Should I back out that change? If the validity of our assumption ever changes at some point in the future we can modify the test again.

@bkline

bkline Apr 24, 2017

Contributor

Actually, looking back at http://www.xmlsoft.org/html/libxml-xmlerror.html#xmlError I think that was a mistake. I had read the comment earlier about the possibility that the parser context not be available ("the parser context if available"), and I mis-remembered that as applying to the xmlNode pointer as well, but looking back I see that there is no such caveat. We do know that there are xmlError structs floating around which have a NULL node pointer (we're creating some ourselves), but a good case could be made that the test should just assume that the pointer will always be non-NULL for schema validation errors. As you can see, that's how I originally wrote the test. Should I back out that change? If the validity of our assumption ever changes at some point in the future we can modify the test again.

This comment has been minimized.

@bkline

bkline Apr 24, 2017

Contributor

I went ahead and rolled back to the tighter version of the test, with a comment explaining what to do if our assumption about always having a location for schema validation errors is no longer valid at some point in the future.

@bkline

bkline Apr 24, 2017

Contributor

I went ahead and rolled back to the tighter version of the test, with a comment explaining what to do if our assumption about always having a location for schema validation errors is no longer valid at some point in the future.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Apr 24, 2017

Member

Very nice and clean, thanks! I'll be happy to merge this. Could you comment on the test, though?

Member

scoder commented Apr 24, 2017

Very nice and clean, thanks! I'll be happy to merge this. Could you comment on the test, though?

@@ -80,7 +89,7 @@ def test_xmlschema_error_log_path(self):
schema.validate(tree)
tree_path = tree.getpath(tree.findall('b')[1])
error_path = schema.error_log[0].path
self.assertTrue(error_path is None or tree_path == error_path)
self.assertTrue(tree_path == error_path)

This comment has been minimized.

@bkline

bkline Apr 24, 2017

Contributor

Better? :-)

@bkline

bkline Apr 24, 2017

Contributor

Better? :-)

@@ -73,6 +76,7 @@ cdef class _LogEntry:
self.column = error.int2
self._c_message = NULL
self._c_filename = NULL
self._c_path = NULL

This comment has been minimized.

@bkline

bkline Apr 24, 2017

Contributor

I had a question on the mailing list about this. By my reading of the Cython documentation, this initialization to NULL has already been taken care of (same for the previous two assignments, as well as the one of self._c_path in _setGeneric() below). Is this correct? Can these initializations have been safely omitted? I'm not saying we should necessarily take them out. I'm just trying to solidify my understanding of the workings of Cython.

Thanks!

@bkline

bkline Apr 24, 2017

Contributor

I had a question on the mailing list about this. By my reading of the Cython documentation, this initialization to NULL has already been taken care of (same for the previous two assignments, as well as the one of self._c_path in _setGeneric() below). Is this correct? Can these initializations have been safely omitted? I'm not saying we should necessarily take them out. I'm just trying to solidify my understanding of the workings of Cython.

Thanks!

This comment has been minimized.

@scoder

scoder Apr 25, 2017

Member

The object memory is initialised to zero by the allocator, so doing this initialisation in the constructor (specifically in Cython's __cinit__()) would be redundant. Since this is an entirely non-special initialiser method, however, that could potentially also be used to reinitialise an existing object, it's better to assure a completely safe and consistent state. And honestly, setting a couple of bytes of memory to zero unconditionally does not cost us anything at all here.

@scoder

scoder Apr 25, 2017

Member

The object memory is initialised to zero by the allocator, so doing this initialisation in the constructor (specifically in Cython's __cinit__()) would be redundant. Since this is an entirely non-special initialiser method, however, that could potentially also be used to reinitialise an existing object, it's better to assure a completely safe and consistent state. And honestly, setting a couple of bytes of memory to zero unconditionally does not cost us anything at all here.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Apr 25, 2017

Member

Excellent, thanks!

Member

scoder commented Apr 25, 2017

Excellent, thanks!

@scoder scoder merged commit 621d8d9 into lxml:master Apr 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bkline

This comment has been minimized.

Show comment
Hide comment
@bkline

bkline Apr 25, 2017

Contributor

Thanks very much!

Contributor

bkline commented Apr 25, 2017

Thanks very much!

@bkline bkline deleted the bkline:error-location branch Apr 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment