Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8038 )
Change subject: IMPALA-5736: Add impala-shell argument to set default query options ...................................................................... Patch Set 15: (6 comments) I'm lukewarm on the exception hierarchy introduced. I appreciate the desire for clearer errors for the user. However, in the case of exception hierarchies, PEP-008 [0] has this to say: > Design exception hierarchies based on the distinctions that code catching the > exceptions is likely to need, rather than the locations where the exceptions > are raised. In the case of the exception hierarchy introduced in patch set 15, there isn't any catching introduced: both exceptions are raised directly to the shell user. Is it that important, then, to introduce this exception hierarchy at all? Again however, I appreciate the desire to have clearer errors. At a high level, there are three options I see: 1. Completely remove the exception hierarchy introduced. 2. Leave exception hierarchy in place, but trim declarations to be more conventional. 3. Leave the exception hierarchy in place, and leave its declarations in this more "verbose", unconventional state. I'm going to leave review comments guiding toward #2. Happy to hear arguments for 1 or 3. [0] https://www.python.org/dev/peps/pep-0008/ http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@36 PS15, Line 36: class OptionParserError(Exception): : """Base class for option parser exceptions.""" : def __init__(self, msg): : self.msg = msg I think this should be removed. The only reason to have a hierarchy like this is if calling code needs to except the whole group, i.e, except OptionParserError: The calling code isn't catching any of these new exceptions. http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41 PS15, Line 41: FileFormatError ConfigFileFormatError might be a clearer name. http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43 PS15, Line 43: def __init__(self, msg): : self.msg = msg I don't you think you need to override __init__, because the Exception hierarchy already has a builtin message attribute: >>> Exception.message <attribute 'message' of 'exceptions.BaseException' objects> >>> That is, if you use the attribute "message" that already exists instead of rolling your own "msg" attribute, you can make the declaration of this exception cleaner. However, I'm not sure you need to do anything inside this class with "message" or "msg" if you follow the next comment's advice. http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46 PS15, Line 46: def __str__(self): : return "Unable to read configuration file correctly. Check formatting: %s" % self.msg Instead of overriding __str__, it is more conventional to let the message argument when raising the exception be the full message of the exception. Let the message and clear exception class name tell the full story. Taking this comment and the previous one together, this is the more conventional way to create one's own exception: class MyError(ParentException): """docstring""" pass # ... raise MyError("full message of exception") http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@49 PS15, Line 49: InvalidValueError InvalidOptionValueError? http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51 PS15, Line 51: def __init__(self, msg): : self.msg = msg : : def __str__(self): : return "Unexpected value in configuration file. %s" % self.msg Same comments with __init__ and __str__ apply here. -- To view, visit http://gerrit.cloudera.org:8080/8038 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986 Gerrit-Change-Number: 8038 Gerrit-PatchSet: 15 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Wed, 25 Oct 2017 18:22:58 +0000 Gerrit-HasComments: Yes
