You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/04/19 16:44:29 UTC
[GitHub] [nifi] Lehel44 opened a new pull request, #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Lehel44 opened a new pull request, #5977:
URL: https://github.com/apache/nifi/pull/5977
…ableValues
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
https://issues.apache.org/jira/browse/NIFI-9936
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #5977:
URL: https://github.com/apache/nifi/pull/5977#discussion_r859793743
##########
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##########
@@ -396,6 +396,20 @@ public <E extends Enum<E>> Builder allowableValues(final E[] values) {
return this;
}
+ /**
+ * Stores allowable values from an enum class.
+ * @param enumClass an enum class that implements the Allowable interface and contains a set of values
+ * @param <E> generic parameter for an enum class that implements the Allowable interface
+ * @return the builder
+ */
+ public <E extends Enum<E> & DescribedValue> Builder allowableValues(final Class<E> enumClass) {
+ this.allowableValues = new ArrayList<>();
+ for (E enumValue : enumClass.getEnumConstants()) {
+ this.allowableValues.add(new AllowableValue(enumValue.name(), enumValue.getDisplayName(), enumValue.getDescription()));
Review Comment:
Following the interface adjustments, `enumValue.name()` should be replaced with `enumValue.getValue()`:
```suggestion
this.allowableValues.add(new AllowableValue(enumValue.getValue(), enumValue.getDisplayName(), enumValue.getDescription()));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
URL: https://github.com/apache/nifi/pull/5977
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #5977:
URL: https://github.com/apache/nifi/pull/5977#issuecomment-1108576774
> @nandorsoma @exceptionfactory @tpalfy Thanks for the comments. I went over recommended interface names. The PropertyValue already exists so I chose the DescribedValue. I added the getValue() method to the interface, it can be overriden not only for the enum instances, but for the whole class.
>
> `public enum EnumAllowableValue implements DescribedValue {
>
> ```
> GREEN {
> @Override
> public String getDisplayName() {
> return "GreenDisplayName";
> }
>
> @Override
> public String getDescription() {
> return "GreenDescription";
> }
> },
>
> RED {
> @Override
> public String getDisplayName() {
> return "RedDisplayName";
> }
>
> @Override
> public String getDescription() {
> return "RedDescription";
> }
> };
>
> @Override
> public String getValue() {
> return name();
> }
> ```
>
> }`
Interesting solution, but I like it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on a diff in pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on code in PR #5977:
URL: https://github.com/apache/nifi/pull/5977#discussion_r860100082
##########
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##########
@@ -396,6 +396,20 @@ public <E extends Enum<E>> Builder allowableValues(final E[] values) {
return this;
}
+ /**
+ * Stores allowable values from an enum class.
+ * @param enumClass an enum class that implements the Allowable interface and contains a set of values
+ * @param <E> generic parameter for an enum class that implements the Allowable interface
+ * @return the builder
+ */
+ public <E extends Enum<E> & DescribedValue> Builder allowableValues(final Class<E> enumClass) {
+ this.allowableValues = new ArrayList<>();
+ for (E enumValue : enumClass.getEnumConstants()) {
+ this.allowableValues.add(new AllowableValue(enumValue.name(), enumValue.getDisplayName(), enumValue.getDescription()));
Review Comment:
Oh wow, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] Lehel44 commented on pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
Lehel44 commented on PR #5977:
URL: https://github.com/apache/nifi/pull/5977#issuecomment-1108474060
@nandorsoma @exceptionfactory @tpalfy
Thanks for the comments. I went over reommended interface names. The PropertyValue already exists so I went with the DescribedValue. I added the getValue() method to the interface, it can be overriden for the enum whole class.
`public enum EnumAllowableValue implements DescribedValue {
GREEN {
@Override
public String getDisplayName() {
return "GreenDisplayName";
}
@Override
public String getDescription() {
return "GreenDescription";
}
},
RED {
@Override
public String getDisplayName() {
return "RedDisplayName";
}
@Override
public String getDescription() {
return "RedDescription";
}
};
@Override
public String getValue() {
return name();
}
}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #5977:
URL: https://github.com/apache/nifi/pull/5977#discussion_r855497812
##########
nifi-api/src/main/java/org/apache/nifi/components/Allowable.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.components;
+
+public interface Allowable {
Review Comment:
Recommend renaming this interface to `DescribedValue`, which should communicate the purpose more clearly.
##########
nifi-api/src/main/java/org/apache/nifi/components/Allowable.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.components;
+
+public interface Allowable {
+
+ String getDisplayName();
+ String getDescription();
Review Comment:
With these methods being proposed as part of the `nifi-api` module, they should have basic Java documentation indicating the general contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a diff in pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
tpalfy commented on code in PR #5977:
URL: https://github.com/apache/nifi/pull/5977#discussion_r856160347
##########
nifi-api/src/main/java/org/apache/nifi/components/Allowable.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.components;
+
+public interface Allowable {
+
+ String getDisplayName();
+ String getDescription();
+}
Review Comment:
There are already some comments about the name of the interface but I would go a little bit further.
I would elevate it from being a purely technical tool to a domain entity, like this:
```suggestion
public interface PropertyValue {
String getValue();
String getDisplayName();
String getDescription();
}
```
With this the interface represents a real-life concept instead of just being a convenient device.
(This would actually also allow us to use the new interface in the PropertyDescriptor but that would require a more careful analysis.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on a diff in pull request #5977: NIFI-9936: Added enum support in PropertyDescriptor Builder for Allow…
Posted by GitBox <gi...@apache.org>.
markap14 commented on code in PR #5977:
URL: https://github.com/apache/nifi/pull/5977#discussion_r857792910
##########
nifi-api/src/main/java/org/apache/nifi/components/DescribedValue.java:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.components;
+
+/**
+ * Describes a processor property value with display name and description.
Review Comment:
This should probably say a "component property" - this can be used for more than just processors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org