-
Notifications
You must be signed in to change notification settings - Fork 93
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
2982.elegantly handle known errors #2995
2982.elegantly handle known errors #2995
Conversation
f55e5f3
to
d96d81d
Compare
flines = inline( | ||
flines, fdir, fpath, False, viewcfg=viewcfg, for_edit=asedit) | ||
except IncludeFileNotFoundError as exc: | ||
raise FileParseError(str(exc)) |
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.
One example of catching a specific exception then raising a generic one in its place.
return '%s: %s' % (self.__class__.__name__, self.args[0]) | ||
|
||
|
||
class ItemNotFoundError(ParsecError, KeyError): |
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.
Is this a sensible way to provide more context to the exception or a potential hazard?
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 so, knowing it's due/related to KeyError
looks simple and useful.
pass | ||
|
||
|
||
class CyclerTypeError(CyclingError): |
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.
Do we need super-specialised exceptions like this?
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.
Presumably a good reason for a "super-specialised exception" is if the calling code needs to catch and respond specifically to that specific exception; otherwise it's pointless.
Sounds pretty sensible to me, although I wouldn't say I have a deep understanding of exception handling best practice (if there is such a thing). |
Happy with approach. Key points:
|
I like the classes design too! In Java and PHP, normally you have one base exception class, and the children classes are designed per application feature/issue/section/etc. So +1 🎉 As for the decorator From #2972, some libraries that handle command line parsing/arguments also provide ways to intercept and display exceptions, which could be used with or replace the decorator (I think?)
Another possibility, is that in #2802 the current Cylc commands could be re-designed in some simple object structure. Depending on the outcome of that task, we could instead of having each command with a try-catch, or with a decorator, have an entry point that uses the new Python code. aws-cli has an entry point where exception is handled. But not sure if we will go in the same direction. One benefit of having this approach, is that if we had the logic of |
OK, based on this response I'll finish work on this PR, lots of tests to fix!Note, the parsec tests were pretty easy to fix as they are unittests which only check the exception raised not the full text. Test the interface not the implementation is a good motto. |
d96d81d
to
ce8f8d3
Compare
(@kinow - in your comment above, I guess you were suggesting possible ways to do even better in future, not changes to this PR?) |
ce8f8d3
to
6bfc298
Compare
6bfc298
to
4700ff5
Compare
4700ff5
to
81b2c0c
Compare
7c00811
to
44517fd
Compare
44517fd
to
9933644
Compare
9933644
to
3dfff49
Compare
|
@kinow resolved codacy niggles, unified |
b8bb531
to
6a3a255
Compare
Deconflicted. |
All looks good via 👀 with 👓 . |
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.
LGTM so far, but still going through the code. I like the CLI function decorator 😁
6a3a255
to
c8dce24
Compare
Good! This could pick up other functionality like colour settings/parsing. |
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.
Approve pending minor deconfliction.
Note also #3056 adds more SuiteConfigError
.
c8dce24
to
89d4c88
Compare
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.
@oliver-sanders you've rebased onto some recently-merged old-style use of SuiteConfigError (with redundant "ERROR" strings) that need to be made (aesthetically) consistent with your changes here.
$ git status
HEAD detached at oliver/2982.elegantly-handle-known-errors
$ grep -A 1 -B 1 ERROR lib/cylc/config.py
raise SuiteConfigError(
f"ERROR, "
f"xtrigger function not callable: "
raise SuiteConfigError(
f"ERROR, "
f"xtrigger function not found: {xtrig.func_name}")
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.
Re-approving!
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.
🎉
🎉 |
For DiscussionI've made a quick start at #2982, raising as a PR now to discuss approach before committing to it.WIPI've got thetests/validate
tests to pass, these changes should give a flavour for what this would look like if fully implemented.closes #2982
Highlights:
CylcError
andParsecError
respectively.UserImputError
for CLI input.GraphNodeError
in withGraphParseError
CylcError
,ParsecError
errors elegantly but allow anything else to materialise in traceback.CylcError
exceptions in order to re-raise as a more generic type e.g.GraphParseError
being re-raised as aSuiteConfigError
def __str__(self): return repr(self.msg)
.ERROR
prefix with the more informative error class name.ItemError: ERROR: error in item
).Example:
before: catch specific exception and re-raise as a generic exception
after: remove un-necessary
Include-file not found
text, exception title should sufficeTODO:
ISODateTimeError
.Questions
@cylc/core Are we happy with this approach?
Should exceptions multiple-inherit for context (
see lib/parsec/exceptions.py
for example):