Deprecate groovy #611
Deprecate groovy #611
Conversation
Deprecate methods that take explicit groovy types (e.g. groovy.lang.Closure, groovy.time.TimeDuration, etc). Also updates some javadoc that mentioned “Closure” to say that its Usage is deprecated.
aledsage
force-pushed
the
deprecate-groovy
branch
from
1d52e2e
to
1d056d8
Compare
| public static <I,T> ExplicitEffector<I,T> create(String name, Class<T> type, List<ParameterType<?>> parameters, String description, Closure body) { | ||
| LOG.warn("Use of groovy.lang.Closure is deprecated, in ExplicitEffector.create()"); |
There was a problem hiding this comment.
Shouldn't we suggest another class/method to use in the warning message?
There was a problem hiding this comment.
I've added this to javadoc on the class - I think that's good enough, as there's not a direct replacement (i.e. one would extend AbstractEffector yourself instead, as we don't need to workaround for GROOVY-5122 if you're not using groovy - I'm not sure why anyone would want to do that).
I agree in general that if deprecating a method/class, one should point at the alternative - but if deprecating a class, I don't think we need it on every method.
| /** | ||
| * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Shouldn't we add a warning message here?
There was a problem hiding this comment.
My rule-of-thumb... if one might get at the code via a yaml blueprint (so never seeing a deprecation warning in your IDE/compiler), then we should include a log.warn. But if we can assume that the user will get compiler warnings, then that is sufficient.
I believe that this will only get called directly in code, so one should see a compiler warning.
| /** | ||
| * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Yes, the parameter is of type groovy.time.TimeDuration.
|
@tbouron I've responded to your comments - do you agree with those, or do you think I should make further changes before merging the PR? |
|
@aledsage Yes, I think that covers everything so LGTM 👍 |
|
Thanks @tbouron - merging now. |
As discussed on the dev@brooklyn mailing list (subject "[PROPOSAL] Remove groovy dependency/support").
Deprecate methods that take explicit groovy types (e.g.
groovy.lang.Closure,groovy.time.TimeDuration, etc).Also updates some javadoc that mentioned
Closureto say that its usage is deprecated.In some places, it does a
LOG.warn()about the deprecation when a groovy Closure etc is passed in - this is when we can't just deprecate the method, e.g. for use ofTypeCoercionon when the method takesObjectand does an uglyinstanceof.This does not touch:
org.apache.brooklyn.rest.resources.ScriptResource(for groovy-script execution)AbstractManagementContext.invokeEffectorMethodLocal()callingGroovyJavaMethods.invokeMethodOnMetaClass)