You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/05/15 18:30:39 UTC
[GitHub] [commons-configuration] darkma773r opened a new pull request, #182: CONFIGURATION-764: Single Variable Interpolation
darkma773r opened a new pull request, #182:
URL: https://github.com/apache/commons-configuration/pull/182
improving detection of single variable interpolations in ConfigurationInterpolator
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory merged pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
garydgregory merged PR #182:
URL: https://github.com/apache/commons-configuration/pull/182
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
darkma773r commented on PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#issuecomment-1132410108
@garydgregory, do you have any objections to this being merged?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
darkma773r commented on PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#issuecomment-1133766346
> Maybe the isSingleVariable method should document that this is only a simple attempt at optimization?
Unfortunately, this isn't just an optimization, but affects the return value. If the value to interpolate consists of just a single variable, then the variable is looked up and returned with its original type. If it is not a single variable or if the single variable lookup does not return a result, then `StringSubstitutor` is used and the result is a string, regardless of the type of the resolved variable(s). So, it is important to catch as many cases of single variable references as possible so that the expected type can be returned. However, the parsing used by `ConfigurationInterpolator` in the single variable case is not nearly as sophisticated as that used in `StringSubstitutor` -- there is no such thing as default values or escaping in the `ConfigurationInterpolator` logic. This may mean that we're not losing any existing functionality here but I'll need think about this a bit more...
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on a diff in pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#discussion_r873689727
##########
src/main/java/org/apache/commons/configuration2/interpol/ConfigurationInterpolator.java:
##########
@@ -322,14 +322,14 @@ public boolean isEnableSubstitutionInVariables() {
/**
* Checks whether a value to be interpolated seems to be a single variable. In this case, it is resolved directly
- * without using the {@code StringSubstitutor}. Note that it is okay if this method returns a false positive: In this
- * case, resolving is going to fail, and standard mechanism is used.
+ * without using the {@code StringSubstitutor}.
*
* @param strValue the value to be interpolated
* @return a flag whether this value seems to be a single variable
*/
private boolean looksLikeSingleVariable(final String strValue) {
- return strValue.startsWith(VAR_START) && strValue.endsWith(VAR_END);
Review Comment:
Is the check considered strict now? If so, then the method name is no longer appropriate.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#issuecomment-1135803535
@darkma773r
Thank you for your explanation.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on a diff in pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
darkma773r commented on code in PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#discussion_r874324516
##########
src/main/java/org/apache/commons/configuration2/interpol/ConfigurationInterpolator.java:
##########
@@ -322,14 +322,14 @@ public boolean isEnableSubstitutionInVariables() {
/**
* Checks whether a value to be interpolated seems to be a single variable. In this case, it is resolved directly
- * without using the {@code StringSubstitutor}. Note that it is okay if this method returns a false positive: In this
- * case, resolving is going to fail, and standard mechanism is used.
+ * without using the {@code StringSubstitutor}.
*
* @param strValue the value to be interpolated
* @return a flag whether this value seems to be a single variable
*/
private boolean looksLikeSingleVariable(final String strValue) {
- return strValue.startsWith(VAR_START) && strValue.endsWith(VAR_END);
Review Comment:
Yes, it is strict. I've updated the name and the docs.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] garydgregory commented on pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#issuecomment-1133644573
> @garydgregory, do you have any objections to this being merged?
It might be ok but the single variable check will fail if a variable default value contains the end marker (escaped). Maybe the isSingleVariable method should document that this is only a simple attempt at optimization?
The larger issue is that we have interpolation code all over in Apache projects. I'm trying to standardize on the Commons Text implementation but that implementation is not perfect either...
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-configuration] darkma773r commented on pull request #182: CONFIGURATION-764: Single Variable Interpolation
Posted by GitBox <gi...@apache.org>.
darkma773r commented on PR #182:
URL: https://github.com/apache/commons-configuration/pull/182#issuecomment-1135343473
> It might be ok but the single variable check will fail if a variable default value contains the end marker (escaped).
@garydgregory, I haven't been able to find a way to reproduce this. Any time I include a `}` character in the variable name or default value, `StringSubstitutor` uses that as the end of the variable reference. I also do not see this escaping being handled in the `StringSubstitutor` code. Do you have an example?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org