derive EasyBuildError from LoggedException, deprecate log.error in favor of raise EasyBuildError #1218
Conversation
…ldError in log.error/log.exception
|
This is just a proof-of-concept, all other uses of @stdweird, @JensTimmerman, @riccardomurri: thoughts? |
| @@ -94,7 +95,7 @@ def experimental(self, msg, *args, **kwargs): | |||
| self.warning(msg, *args, **kwargs) | |||
| else: | |||
| msg = 'Experimental functionality. Behaviour might change/be removed later (use --experimental option to enable). ' + msg | |||
| self.error(msg, *args) | |||
| raise EasyBuildError(msg % args) | |||
There was a problem hiding this comment.
why not support EasyBuildError(msg, *args) ?
|
Refer to this link for build results (access rights to CI server needed): |
|
remarks fixed, pls rereview @stdweird? |
|
Refer to this link for build results (access rights to CI server needed): |
| class EasyBuildError(Exception): | ||
| # 1st argument (a LoggedException instance) is ignored | ||
| def log_error_no_raise(_, logger, msg): | ||
| """Utility function to log an error with raising an exception.""" |
There was a problem hiding this comment.
thsi method is deprected and add _ to indicate it's private
|
Refer to this link for build results (access rights to CI server needed): |
|
remarks fixed, please revisit before I tackle all remaining occurences of |
| orig_raise_error = self.raiseError | ||
| self.raiseError = False | ||
|
|
||
| self.deprecated("Use of dedicated _error_no_raise log method", '3.0') |
There was a problem hiding this comment.
I may have to revisit this bit, since the tests treat all log.deprecated statements being hit as errors (by defining EASYBUILD_DEPRECATED to same ridiculously high version)
I don't think that'll work out well (since we'll be hitting this one all the time). Not sure where I can move this though for proper deprecation of _error_no_raise...
There was a problem hiding this comment.
After giving this some more thought, I don't think we should be using log.deprecated at all here...
The idea behind the log.deprecated is to indicate that deprecated functionality, i.e. stuff that will be removed in the future, is being triggered.
In this case _error_no_raise actually implement the behaviour that we want to be using, so it shouldn't trigger log.deprecated at all... Leaving it like this, we'll be triggering log.deprecated whenever an EasyBuildError is created.
I'll see if I can refurbish things to log.error being the behaviour we want (no raise), and move the bit currently in error that does the raise in a separate method that does contain log.deprecated.
I'm not sure that will fly though, since by default we actually want log.error to still raise, for now (to retain backward-compatibility).
There was a problem hiding this comment.
then simply add comment that indicates that method should never be used and that we are going to remove this in 3.0. no need to hack more around. it's already bad enough 😄
|
Refer to this link for build results (access rights to CI server needed): |
| self.msg = msg | ||
|
|
||
| def __str__(self): | ||
| """Return string representation of this EasyBuildError instance.""" | ||
| print '__str__' |
|
Refer to this link for build results (access rights to CI server needed): |
|
(GitHub was having some problems at the time the tests ran, so I've retriggered them) |
|
Refer to this link for build results (access rights to CI server needed): |
|
and again... Jenkins: test this please |
|
Jenkins: test this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
Jenkins: test this please |
|
documentation has been updated accordingly, see http://easybuild.readthedocs.org/en/latest/Deprecated-functionality.html#report-errors-by-raising-easybuilderror-rather-than-using-log-methods |
|
Refer to this link for build results (access rights to CI server needed): |
|
Jenkins: test this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Jenkins: test this please |
2 similar comments
|
Jenkins: test this please |
|
Jenkins: test this please |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| orig_raise_error = self.raiseError | ||
| self.raiseError = False | ||
|
|
||
| self.error(msg) |
There was a problem hiding this comment.
I need to change this using Fancylogger.error, otherwise errors are reporting in the logs like:
== 2015-03-31 13:34:58,605 main ERROR EasyBuild crashed with an error
(at easybuild/tools/build_log.py:136 in _error_no_raise):
Can't find path /Users/kehoste/work/easybuild-framework/foo.eb
There was a problem hiding this comment.
while it used to be like:
ERROR: EasyBuild crashed with an error (at easybuild/framework/easyconfig/tools.py:317 in parse_easyconfigs):
Can't find path /Users/kehoste/work/easybuild-framework/foo.eb
There was a problem hiding this comment.
I'll handle this in a follow-up PR.
|
That's it, I'm merging in this beast. Progress! Thanks for the review @stdweird! |
derive EasyBuildError from LoggedException, deprecate log.error in favor of raise EasyBuildError
replaces #1183
depends on
hpcugent/vsc-base#160hpcugent/vsc-base#163update: see http://easybuild.readthedocs.org/en/latest/Deprecated-functionality.html#report-errors-by-raising-easybuilderror-rather-than-using-log-methods