You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/03/24 18:21:35 UTC
[GitHub] brooklyn-server pull request #611: Deprecate groovy
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/611
Deprecate groovy
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 `Closure` to 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 of `TypeCoercion` on when the method takes `Object` and does an ugly `instanceof`.
This does not touch:
* The `org.apache.brooklyn.rest.resources.ScriptResource` (for groovy-script execution)
* Internal usage of groovy (e.g. `AbstractManagementContext.invokeEffectorMethodLocal()` calling `GroovyJavaMethods.invokeMethodOnMetaClass`)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server deprecate-groovy
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/611.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #611
----
commit dea3efca3238aab52564e816dc572686b63f3150
Author: Aled Sage <al...@gmail.com>
Date: 2017-03-24T18:04:45Z
Deprecate groovy methods
Deprecate methods that take explicit groovy types (e.g. groovy.lang.Closure,
groovy.time.TimeDuration, etc).
Also updates some javadoc that mentioned \u201cClosure\u201d to say that its
Usage is deprecated.
commit 1d52e2e468bdf3bf83ec1b61279c197577fb3e10
Author: Aled Sage <al...@gmail.com>
Date: 2017-03-24T18:15:38Z
Deprecate passing groovy to Brooklyn Main
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #611: Deprecate groovy
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/611
@aledsage Yes, I think that covers everything so LGTM \U0001f44d
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108374329
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcessImpl.java ---
@@ -476,6 +476,11 @@ public void waitForServiceUp() {
public void waitForServiceUp(Duration duration) {
Entities.waitForServiceUp(this, duration);
}
+
+ /**
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
+ */
+ @Deprecated
--- End diff --
Is this one really deprecated?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108627254
--- Diff: core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java ---
@@ -133,6 +133,10 @@ public MethodEffector(Method method) {
this(new AnnotationsOnMethod(method.getDeclaringClass(), method), null);
}
+ /**
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
+ */
+ @Deprecated
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #611: Deprecate groovy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/611
@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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/611
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108372912
--- Diff: core/src/main/java/org/apache/brooklyn/core/effector/ExplicitEffector.java ---
@@ -46,12 +58,20 @@ public T call(Entity entity, Map parameters) {
/** convenience to create an effector supplying a closure; annotations are preferred,
* and subclass here would be failback, but this is offered as
- * workaround for bug GROOVY-5122, as discussed in test class CanSayHi
+ * workaround for bug GROOVY-5122, as discussed in test class CanSayHi.
+ *
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
*/
+ @Deprecated
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()");
--- End diff --
Shouldn't we suggest another class/method to use in the warning message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #611: Deprecate groovy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/611
Thanks @tbouron - merging now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108373185
--- Diff: core/src/main/java/org/apache/brooklyn/core/effector/MethodEffector.java ---
@@ -133,6 +133,10 @@ public MethodEffector(Method method) {
this(new AnnotationsOnMethod(method.getDeclaringClass(), method), null);
}
+ /**
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
+ */
+ @Deprecated
--- End diff --
Shouldn't we add a warning message here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108627523
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/SoftwareProcessImpl.java ---
@@ -476,6 +476,11 @@ public void waitForServiceUp() {
public void waitForServiceUp(Duration duration) {
Entities.waitForServiceUp(this, duration);
}
+
+ /**
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
+ */
+ @Deprecated
--- End diff --
Yes, the parameter is of type `groovy.time.TimeDuration`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108634563
--- Diff: core/src/main/java/org/apache/brooklyn/core/effector/ExplicitEffector.java ---
@@ -46,12 +58,20 @@ public T call(Entity entity, Map parameters) {
/** convenience to create an effector supplying a closure; annotations are preferred,
* and subclass here would be failback, but this is offered as
- * workaround for bug GROOVY-5122, as discussed in test class CanSayHi
+ * workaround for bug GROOVY-5122, as discussed in test class CanSayHi.
+ *
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
*/
+ @Deprecated
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()");
--- End diff --
Fair enough
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #611: Deprecate groovy
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/611#discussion_r108627021
--- Diff: core/src/main/java/org/apache/brooklyn/core/effector/ExplicitEffector.java ---
@@ -46,12 +58,20 @@ public T call(Entity entity, Map parameters) {
/** convenience to create an effector supplying a closure; annotations are preferred,
* and subclass here would be failback, but this is offered as
- * workaround for bug GROOVY-5122, as discussed in test class CanSayHi
+ * workaround for bug GROOVY-5122, as discussed in test class CanSayHi.
+ *
+ * @deprecated since 0.11.0; explicit groovy utilities/support will be deleted.
*/
+ @Deprecated
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()");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---