Csaba Ringhofer 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 16: (6 comments) > (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/ I am unsure about 1 vs 2. Probably no one will ever differentiate between these exceptions, so they could be simply thrown as Exception. I have made the patch according to your comments towards 2. 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 ConfigFileFormatError(Exception): : """Raised when the config file cannot be read by ConfigParser.""" : pass : > I think this should be removed. The only reason to have a hierarchy like th Done http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@41 PS15, Line 41: aised when an o > ConfigFileFormatError might be a clearer name. Done http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@43 PS15, Line 43: : def parse_bool_opt > I don't you think you need to override __init__, because the Exception hier Done http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@46 PS15, Line 46: Throws ValueError for other values. : """ > Instead of overriding __str__, it is more conventional to let the message a Done http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@49 PS15, Line 49: turn True > InvalidOptionValueError? Done http://gerrit.cloudera.org:8080/#/c/8038/15/shell/option_parser.py@51 PS15, Line 51: return False : else: : raise InvalidOptionValueError("Unexpected value in configuration file. '" + value \ : + "' is not a valid value for a boolean option.") : > Same comments with __init__ and __str__ apply here. Done -- 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: 16 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Mon, 30 Oct 2017 21:51:14 +0000 Gerrit-HasComments: Yes
