Skip to content
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.

Sign up for GitHub

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

Jump to bottom

catch Pyparsing exceptions #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

catch Pyparsing exceptions #10

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5296fe8
add flag parsed=true for sendExpression
arun3688 Apr 11, 2016
8c0f91d
catch PyParsing Exception results
arun3688 Apr 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions OMPython/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import time
import logging
import uuid
import getpass
import subprocess
import tempfile
import pyparsing
Expand Down Expand Up @@ -182,7 +183,7 @@ def __init__(self, readonly=False):
if sys.platform == 'win32':
self._omc_log_file = open(os.path.join(self._temp_dir, "openmodelica.objid." + self._random_string+".log"), 'w')
else:
self._currentUser = os.environ['USER']
self._currentUser = getpass.getuser()
if not self._currentUser:
self._currentUser = "nobody"
# this file must be closed in the destructor
Expand Down Expand Up @@ -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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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.

Copy link
Collaborator Author

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 <notifications@github.com

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:
  1. 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.
  2. 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

Copy link

@bilderbuchi bilderbuchi Apr 29, 2016 •
edited
Loading

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.

Copy link
Collaborator Author

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.

Copy link

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).

Copy link
Member

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.

Copy link
Member

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.

Copy link
Collaborator Author

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

Copy link
Member

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.

return result
else:
return "No connection with OMC. Create an instance of OMCSession."

Expand Down