DM-9846: Improve handling of error messages #4

r-owen · 2017-03-16T21:44:12Z

No description provided.

kfindeisen · 2017-03-17T15:27:02Z

src/base.cc

+    // Construct ErrorHandler once, the first time this function is called.
+    // This is done to initialize `errorMsgStream` and register `reportError` as the AST error handler.
+    // See https://isocpp.org/wiki/faq/ctors#static-init-order
+    static ErrorHandler * errHandler = new ErrorHandler();


EDIT: Never mind, found https://isocpp.org/wiki/faq/ctors#construct-on-first-use-v2.

Perhaps I should change the link to https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use (which is the solution, and comes right before the discussion about why to use new.

Also, it may not be safe just rely on a link, as it may go stale. @jonathansick do we have a way to archive a useful page so we're sure we always have access to it? I'd hate to copy all that information to this note.

I don't think you need to copy all that information. The static initialization order issue is fairly well known (e.g., there's an item devoted to it in Effective C++), so a brief mention should be all the justification you need. The static de-initialization issue is not one I was familiar with, but it's exactly the same principle and so can probably be explained in one line.

@r-owen We don't have a formal system for that, but it's an interesting concept. Probably combining ls.st with an auto-archive service that fails over if the original link 404s. I'll put that in my idea bucket. Something probably exists off-the-shelf for this. In the meantime, webarchive.org can save us if the link goes stale.

kfindeisen

No major problems, just some style nitpicks.

kfindeisen · 2017-03-21T18:23:54Z

doc/mainpage.dox

-Longer term desires:
- Support AST regions
- Add support for building with cmake.
- Add doc strings to python wrapped objects.


Did you mean to remove the "longer-term" bit as well?

Yes. The current code will probably suffice for LSST and it's not clear to me which, if any, additional AST features we will want to add. Much as I would like python doc strings I suspect it is not practical unless we come up with an automated way to add them.

kfindeisen · 2017-03-21T18:53:41Z

src/base.cc

+        ErrorHandler() {
+            astSetPutErr(reportError);
+        }
+


Is astshim bound by RFC-209? I'm not clear on its relationship to the Stack.

It is not bound but I have been doing that for most classes. It seems fussy for such a tiny class, but I'll add it.

kfindeisen · 2017-03-21T18:54:28Z

src/base.cc

+            astSetPutErr(reportError);
+        }
+
+        std::string getErrMsg() {


Can getErrMsg() be made static?

kfindeisen · 2017-03-21T18:56:11Z

src/base.cc

+        std::string getErrMsg() {
+            std::string errMsg = errorMsgStream.str();
+            errorMsgStream.clear();
+            if (errMsg.empty()) {


Should clear() be called even if errMsg is empty?

I believe so. clear clears state flags, such as the error bit, so it is probably always best to call that. To empty the buffer requires calling str("") and I think I will do that only if a string was read. I added a test to test_object.py that makes sure the buffer is properly cleared.

timj · 2017-03-21T19:14:50Z

Does this change make the AST error code itself available to the code that is catching the exception? It seems a bit lossy to end up with a RuntimeError that can not be used to work out what really went wrong without looking at the text of the error message.

kfindeisen · 2017-03-21T20:08:29Z

That would be a nice feature, but I think it would require a custom exception. Perhaps as a separate ticket?

timj · 2017-03-21T20:19:44Z

Something that's pretty easy in Python but I don't know how to do it in C++ so I'll leave the decision on that to the experts. I think it would be something that we should definitely have in the future though.

r-owen · 2017-03-22T12:19:44Z

@timj Including the error code as a value in the exception would be a lot more work and I consider it out of scope for this ticket. We could easily print the AST status as part of the error message but I'm reluctant to do so without a demonstrated need; so far I have found the error messages perfectly clear by themselves.

Using the new AST function `astSetPutErr` write AST error messages to an ostringstream and have `assertOK` use the contents of that stream as the error message when it raises an exception.

python/astshim/keyMap.cc was moved into a subdirectory, but somehow the original was still present (though ignored).

test_object.py was trying to call methods that were no longer publicly available. This passed because it was checking for errors (just not the ones being raised). Improve test_channel.py and test_normMap.py to test for a more explicit exception when using assertRaises. I checked the usage in other tests and concluded it was best to leave them as is because in some cases it wasn't clear which C++ exception would be mapped to the Python exception that was raised, and in others it was unclear if AST or C++ code would catch the problem, thus which Python exception would be raised.

...about comparing unsigned int to int

timj · 2017-03-22T15:51:39Z

Ok. I just feel uncomfortable throwing a runtime error for every possible AST problem. In pyast we go the other extreme and have a special exception for every AST error: https://github.com/timj/starlink-pyast/blob/master/tools/make_exceptions.py but it's probably the case that there should be groups of related error types triggering ValueError vs TypeError etc. I don't want to derail this PR but please consider in the future how you are catching exceptions and whether more granularity would lead to cleaner code.

kfindeisen · 2017-03-24T17:15:16Z

@timj, @r-owen: this may be a stupid question, but are AST's error codes documented anywhere (they don't seem to be in the AST Programmer's Guide)? Adding the error code to an exception is rather pointless if a program can't do anything with the information.

timj · 2017-03-24T17:23:00Z

The source of truth is at https://github.com/Starlink/ast/blob/master/ast_err.msg

I concur that the API document does not actually say which error codes are relevant for which method. Things like AST__ATTIN are equivalent to Python ValueError. @dsberry do you want to comment on this?

Most of the error codes are fatal, so I guess that a program will not need to care about specifics in those cases and the error message is fine. Some of them are recoverable (such as AST__VSMAL). It would be nice if the error codes were grouped into related exceptions but we didn't try that in pyast.

kfindeisen · 2017-03-24T17:27:25Z

"Which error codes are relevant" is actually ill-defined on a method by method basis, because all methods share the same error flag. My question was more along the lines of "If I find out the error code was 42, what does that actually tell me?"

timj · 2017-03-24T17:33:08Z

I'm not sure I understand you. If you ask for an attribute from an AST object that does not exist AST will trigger AST__ATGER or AST__BADAT. In Python you'd expect that to throw an exception that inherits from KeyError so you can trap it. You don't ever write "42" in your code. In C you would check to see if the error code was set to AST__ATGER (defined in ast_err.h) and respond accordingly. If you always get RuntimeError you can't make any decisions in the code and must always punt to the user to interpret the error message.

r-owen · 2017-03-24T19:03:46Z

astshim attempts to raise appropriate exceptions. The python wrapper needs a bit of work on that, but the C++ is probably OK.

kfindeisen reviewed Mar 17, 2017

View reviewed changes

kfindeisen approved these changes Mar 21, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-9846 branch 4 times, most recently from 384f04f to 228e0b7 Compare March 22, 2017 12:15

r-owen force-pushed the tickets/DM-9846 branch from 228e0b7 to 7d257d3 Compare March 22, 2017 14:00

r-owen added 4 commits March 22, 2017 07:33

Improve error handling

5aa488c

Using the new AST function `astSetPutErr` write AST error messages to an ostringstream and have `assertOK` use the contents of that stream as the error message when it raises an exception.

Remove obsolete file

a35900e

python/astshim/keyMap.cc was moved into a subdirectory, but somehow the original was still present (though ignored).

Fix two compiler warnings

f375f38

...about comparing unsigned int to int

r-owen force-pushed the tickets/DM-9846 branch from 7d257d3 to f375f38 Compare March 22, 2017 14:33

r-owen merged commit f375f38 into master Mar 22, 2017

ktlim deleted the tickets/DM-9846 branch August 25, 2018 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-9846: Improve handling of error messages #4

DM-9846: Improve handling of error messages #4

r-owen commented Mar 16, 2017

kfindeisen Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited

kfindeisen Mar 17, 2017

jonathansick Mar 17, 2017

kfindeisen left a comment

kfindeisen Mar 21, 2017

r-owen Mar 22, 2017 •

edited

kfindeisen Mar 21, 2017

r-owen Mar 22, 2017 •

edited

kfindeisen Mar 21, 2017

kfindeisen Mar 21, 2017

r-owen Mar 22, 2017 •

edited

timj commented Mar 21, 2017

kfindeisen commented Mar 21, 2017

timj commented Mar 21, 2017

r-owen commented Mar 22, 2017

timj commented Mar 22, 2017

kfindeisen commented Mar 24, 2017 •

edited

timj commented Mar 24, 2017

kfindeisen commented Mar 24, 2017

timj commented Mar 24, 2017

r-owen commented Mar 24, 2017

DM-9846: Improve handling of error messages #4

DM-9846: Improve handling of error messages #4

Conversation

r-owen commented Mar 16, 2017

kfindeisen Mar 17, 2017 • edited

Choose a reason for hiding this comment

r-owen Mar 17, 2017 • edited

Choose a reason for hiding this comment

kfindeisen Mar 17, 2017

Choose a reason for hiding this comment

jonathansick Mar 17, 2017

Choose a reason for hiding this comment

kfindeisen left a comment

Choose a reason for hiding this comment

kfindeisen Mar 21, 2017

Choose a reason for hiding this comment

r-owen Mar 22, 2017 • edited

Choose a reason for hiding this comment

kfindeisen Mar 21, 2017

Choose a reason for hiding this comment

r-owen Mar 22, 2017 • edited

Choose a reason for hiding this comment

kfindeisen Mar 21, 2017

Choose a reason for hiding this comment

kfindeisen Mar 21, 2017

Choose a reason for hiding this comment

r-owen Mar 22, 2017 • edited

Choose a reason for hiding this comment

timj commented Mar 21, 2017

kfindeisen commented Mar 21, 2017

timj commented Mar 21, 2017

r-owen commented Mar 22, 2017

timj commented Mar 22, 2017

kfindeisen commented Mar 24, 2017 • edited

timj commented Mar 24, 2017

kfindeisen commented Mar 24, 2017

timj commented Mar 24, 2017

r-owen commented Mar 24, 2017

kfindeisen Mar 17, 2017 •

edited

r-owen Mar 17, 2017 •

edited

r-owen Mar 22, 2017 •

edited

r-owen Mar 22, 2017 •

edited

r-owen Mar 22, 2017 •

edited

kfindeisen commented Mar 24, 2017 •

edited