-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix Logger Parameter with Ops #319
base: master
Are you sure you want to change the base?
Conversation
LoggerPreprecessor is no longer needed, because Context does the Logger injection. But it's still important to test, if the Logger gets confortable injected when using CommandService.
I think this will wreak havoc with the ops matcher, unfortunately. Even though the Logger is optional, it is still a parameter, unlike LogService which is ignored by the ops matching framework. The Logger should be completely invisible to ops, and never appear in any op signature. |
Also, we do still need the LoggerPreprocessor, for scripts, since a script could have a Logger parameter, and the context injection will not handle it. This is the same reason we have the ServicePreprocessor still. |
I have a different opinion, I want the Logger to be an optional parameter. Let's assume: The caller of an op has a sophisticated UI with an integrated logging panel. The logging panel should show the log messages of the op. This can be achieved by providing a prepared Logger as parameter to the op.
Ok, I will restore the LoggerPreprocessor |
OK. There will be consequences though, is all I am saying. I assume you want existing ops that use
These ops will all need their namespace methods updated to include the |
Ok, the consequences your are describing clearly call for a better solution, than what I suggested. |
CC @gselzer: next year when we take stock of the current Ops progress, let's discuss this |
53b6733
to
3dc99c9
Compare
Previously the LoggerPreprocessor automatically injected Logger Paramters for CommandService.
But LoggerPreprocessor didn't work for Ops.
This PR fixes this issue:
The issue is described in http://forum.imagej.net/t/using-the-new-scijava-logging-and-status-service-system-in-an-application/9964/10