-
Notifications
You must be signed in to change notification settings - Fork 58
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
catch Pyparsing exceptions #10
base: master
Are you sure you want to change the base?
Conversation
try: | ||
answer = OMTypedParser.parseString(result) | ||
return answer | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It's typically bad python style to catch all exceptions without qualification, you should probably limit this to PyPArsingExceptions or whatever the type of the exceptions you expect are.
- In the case of the exception, shouldn't you return some info about the exception, e.g. its message? You just return the string you wanted to parse in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look into the code, the results of the compiler is returned not the
string we wanted to parse, because the Pyparsing does not give proper error
messages in case there are errors in the code
regards
arun
On Fri, Apr 29, 2016 at 2:37 PM, Christoph Buchner <[email protected]
wrote:
In OMPython/init.py
#10 (comment):@@ -238,8 +239,11 @@ def sendExpression(self, command):
self._omc = None
return result
else:
answer = OMTypedParser.parseString(result)
return answer
try:
answer = OMTypedParser.parseString(result)
return answer
except:
- It's typically bad python style to catch all exceptions without
qualification, you should probably limit this to PyPArsingExceptions or
whatever the type of the exceptions you expect are.- In the case of the exception, shouldn't you return some info about
the exception, e.g. its message? You just return the string you wanted to
parse in the first place.—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/OpenModelica/OMPython/pull/10/files/8c0f91d26d68440d1fe8c8825124e58cdf39100c#r61570107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this is then just confusing nomenclature. this code snippet tries to parse a string called result
. Let's consider this an input of this snippet, or "the string you want to parse". The string is parsed by passing it to a function called parseString
. What you want to return is an answer
, let's consider this an output of this snippet.
Now, if parseString
throws an exception, instead of reporting or returning the exception message, you return result
, i.e. the input of your snippet. You drop all notice of an error having occured, and return an unparsed input, instead of a parsed output.
Btw, I don't know how familiar you are with python, but throwing exception is typically considered "proper error reporting", so the exceptions should contain valuable error information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you are not getting what i am saying, The result is output message obtained from the complier, (it can be both correct output and also error message). The Pyparsing in few cases does not give error messages, i hope you are using OMPython try this
omc.sendExpression("model a end a;")
output- (a,)
omc.sendExpression("model a end a2;")
The output should be a proper error message returned from the compiler and not just the pyparsing exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so you are saying that if parseString throws an exception, you assume it's because result
contains only a compiler error message, and not because of otherwise malformed compiler output, so you return the compiler error message instead of the parsed answer
, right?
Unfortunately, I'm on Python 3, so not using OMPython (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should catch PyPArsingExceptions instead of all exceptions and should print its message.
Returning result
is just fine. Basically from this function we want to return the result of the OMC API call done via sendExpression
. OMTypedParser.parseString
is used to format the result in a proper python type instead of just returning string type. So if parseString
is successful return the formatted result i.e., answer
otherwise just return the result
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, returning result
is not fine. How do you distinguish between a function returning a string and a function giving an error? If you wanted just the result, just do a call that does not expect the function to be possible to parse. Or add another OMPython call that tries to parse the string and otherwise always returns the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about adding a parsed FLag in send Expression , so that when i want only unparsed result i can specify the flag to be false , like in this commit arun3688@ee3287a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use a different python call for parsed=False. It should also fail when failing to parse, no questions asked.
this PR has conflicts now which need to be solved before it becomes mergeable. |
No description provided.