Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1854691771 Thank you for taking on the merge process and adjusting the Migration Guidance, @exceptionfactory. Have a nice week. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
exceptionfactory closed pull request #8102: NIFI-12452 Improve support for DescribedValue URL: https://github.com/apache/nifi/pull/8102 -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1843684252 I just rebased & squashed the commits, @exceptionfactory. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1843250321 Thank you for adjusting the migration guide @exceptionfactory. I'll try to land the rebased PR in a few hours time. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1843243596 Sounds reasonable to me. Thank you both @exceptionfactory & @markap14 for your valuable feedback. From my understanding the current PR proposal with the overload of `defaultValue` corresponds to the desired approach then. Could one of you take on the task to enhance the [migration guide](https://cwiki.apache.org/confluence/plugins/servlet/mobile?contentId=57905503#MigrationGuidance-Migratingto2.0.0-M1) then? As far as I'm aware sole contributors cannot propose or make changes to Confluence? I've seen that there are conflicts with `main` now. I assume this is due to `HttpNotificationService` being removed by #8104. Should I rebase and force-push this PR to be compatible again or should I let the commiter resolve the issue? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
markap14 commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1841666326 My preference is to keep the name `defaultValue` and add the override. I would avoid allowing for a `defaultValue` that accepts an `Object`. While this could be a change that breaks backward compatibility for a few, I think it's a pretty niche concern and still fair game for a 2.0, as long as it's mentioned in the Migration Guide. Also of note, custom code could be updated to use `.defaultValue( (String) null )` even in 1.x so there would be no need to maintain two different versions of the code, etc. So I think it's fair game. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1839332687 Yes, that's what I was concerned about and why I didn't use an overload in the first draft. If we do not backport this change, maybe we can introduce this as a breaking change for 2.x and add an deprecation for calling the function with `null` in 1.x? As far as I understand there is no reason to call it with `null` anyway, as it does nothing. The only use case I can image where calling it with `null` makes sense is to reset a default value copied from a different PropertyDescriptor. However, that isn't what's happening. Maybe we should add a `clearDefaultValue` for that, similar to the existing methods for other fields. While accepting an `Object` avoids the breaking change, I think it makes the interface less easy to understand. Personally I prefer strongly (tightly) types APIs where applicable. I would prefer the deprecation approach. If that's not feasible I'd rather go with the different name than accepting any type, but that's just my gut feeling. What are your thoughts on this @markap14? -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1837946154 Thank you for your feedback @exceptionfactory and @markap14. I renamed `defaultDescribedValue(DescribedValue)` to `defaultValue(DescribedValue)` making it an overload of the existing `defaultValue(String)`. In this process I also removed all calls to `defaultValue` that statically provide `null` I could find in the codebase. All these calls were effectively doing nothing, as the existing implementation of `defaultValue` just returns the `PropertyDescriptor.Builder` without any modification upon receiving `null`. I assumed this is a more straightforward change than keeping the calls but as `defaultValue((String) null)`. -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
markap14 commented on code in PR #8102:
URL: https://github.com/apache/nifi/pull/8102#discussion_r1413205387
##
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##
@@ -313,6 +313,27 @@ public Builder defaultValue(final String value) {
return this;
}
+/**
+ * Specifies the initial value and the default value that will be used
+ * if the user does not specify a value. When {@link #build()} is
+ * called, if Allowable Values have been set (see
+ * {@link #allowableValues(AllowableValue...)})
+ * and the "Value" of the {@link DescribedValue} object is not
+ * the "Value" of one of those Allowable Values, an Exception will be
thrown.
+ * If the Allowable Values have been set using the
+ * {@link #allowableValues(AllowableValue...)} method, the default
value
+ * should be set providing the {@link AllowableValue} to this method.
+ *
+ * @param describedValue default value holder
+ * @return the builder
+ */
+public Builder defaultDescribedValue(final DescribedValue
describedValue) {
Review Comment:
I definitely think it's fine to require `.defaultValue((String) null)`. But
I'm not sure this is ever needed, since `null` is always the default unless
otherwise specified.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on code in PR #8102:
URL: https://github.com/apache/nifi/pull/8102#discussion_r1412736232
##
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##
@@ -313,6 +313,27 @@ public Builder defaultValue(final String value) {
return this;
}
+/**
+ * Specifies the initial value and the default value that will be used
+ * if the user does not specify a value. When {@link #build()} is
+ * called, if Allowable Values have been set (see
+ * {@link #allowableValues(AllowableValue...)})
+ * and the "Value" of the {@link DescribedValue} object is not
+ * the "Value" of one of those Allowable Values, an Exception will be
thrown.
+ * If the Allowable Values have been set using the
+ * {@link #allowableValues(AllowableValue...)} method, the default
value
+ * should be set providing the {@link AllowableValue} to this method.
+ *
+ * @param describedValue default value holder
+ * @return the builder
+ */
+public Builder defaultDescribedValue(final DescribedValue
describedValue) {
Review Comment:
Yes, this was my initial idea as well. However, this prohibits the user of
`.defaultValue(null)` as the compiler cannot differentiate in that case.
Instead users would need to provide something along the lines of
`.defaultValue((String) null)`.
If we're okay with that, I'm more than happy to make it an overloaded
method. What do you think?
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins commented on code in PR #8102:
URL: https://github.com/apache/nifi/pull/8102#discussion_r1412736232
##
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##
@@ -313,6 +313,27 @@ public Builder defaultValue(final String value) {
return this;
}
+/**
+ * Specifies the initial value and the default value that will be used
+ * if the user does not specify a value. When {@link #build()} is
+ * called, if Allowable Values have been set (see
+ * {@link #allowableValues(AllowableValue...)})
+ * and the "Value" of the {@link DescribedValue} object is not
+ * the "Value" of one of those Allowable Values, an Exception will be
thrown.
+ * If the Allowable Values have been set using the
+ * {@link #allowableValues(AllowableValue...)} method, the default
value
+ * should be set providing the {@link AllowableValue} to this method.
+ *
+ * @param describedValue default value holder
+ * @return the builder
+ */
+public Builder defaultDescribedValue(final DescribedValue
describedValue) {
Review Comment:
Yes, this was my initial idea as well. However, this prohibits the user of
`.defaultValue(null)` as the compiler cannot differentiate in that case.
If we're okay with that, I'm more than happy to make it an overloaded
method. What do you think?
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
exceptionfactory commented on code in PR #8102:
URL: https://github.com/apache/nifi/pull/8102#discussion_r1412706544
##
nifi-api/src/main/java/org/apache/nifi/components/PropertyDescriptor.java:
##
@@ -313,6 +313,27 @@ public Builder defaultValue(final String value) {
return this;
}
+/**
+ * Specifies the initial value and the default value that will be used
+ * if the user does not specify a value. When {@link #build()} is
+ * called, if Allowable Values have been set (see
+ * {@link #allowableValues(AllowableValue...)})
+ * and the "Value" of the {@link DescribedValue} object is not
+ * the "Value" of one of those Allowable Values, an Exception will be
thrown.
+ * If the Allowable Values have been set using the
+ * {@link #allowableValues(AllowableValue...)} method, the default
value
+ * should be set providing the {@link AllowableValue} to this method.
+ *
+ * @param describedValue default value holder
+ * @return the builder
+ */
+public Builder defaultDescribedValue(final DescribedValue
describedValue) {
Review Comment:
Instead of naming this `defaultDescribedValue()`, using `defaultValue()`
seems sufficient as the `DescribedValue` argument type differentiates it from
the String-based method.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] NIFI-12452 Improve support for DescribedValue [nifi]
Lehel44 commented on PR #8102: URL: https://github.com/apache/nifi/pull/8102#issuecomment-1836959793 Thanks @EndzeitBegins for the improvement! I will review -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] NIFI-12452 Improve support for DescribedValue [nifi]
EndzeitBegins opened a new pull request, #8102: URL: https://github.com/apache/nifi/pull/8102 # Summary Improves usability of `DescribedValue` for property declaration, during tests and property value interpretation. Makes exemplary use of new / modified APIs for `ListenHTTP`. [NIFI-12452](https://issues.apache.org/jira/browse/NIFI-12452) # Tracking Please complete the following tracking steps prior to pull request creation. ### Issue Tracking - [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created ### Pull Request Tracking - [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-0` - [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-0` ### Pull Request Formatting - [x] Pull Request based on current revision of the `main` branch - [x] Pull Request refers to a feature branch with one commit containing changes # Verification Please indicate the verification steps performed prior to pull request creation. ### Build - [x] Build completed using `mvn clean install -P contrib-check` - [x] JDK 21 ### Licensing - [x] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html) - [x] New dependencies are documented in applicable `LICENSE` and `NOTICE` files ### Documentation - [x] Documentation formatting appears as expected in rendered files -- 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-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
