garydgregory commented on code in PR #341:
URL: https://github.com/apache/commons-text/pull/341#discussion_r930993240
##########
src/changes/changes.xml:
##########
@@ -78,6 +78,7 @@ The <action> type attribute can be add,update,fix,remove.
<action issue="TEXT-185" type="add" dev="ggregory" due-to="Larry West,
Gary Gregory">Release Notes page hasn't been updated for 1.9 release
yet.</action>
<action type="add" dev="ggregory" due-to="Gary
Gregory">Add StrBuilder.isNotEmpty().</action>
<!-- UPDATE -->
+ <action type="update" dev="mattjuntunen">Make default
string lookups configurable via system property. Remove dns, url, and script
lookups from defaults. If these lookups are required for use in
StringSubstitutor.createInterpolator(), they must be enabled via system
property. See StringLookupFactory for details.</action>
Review Comment:
Move this to the FIX section IMO
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -471,7 +483,7 @@ public void addDefaultStringLookups(final Map<String,
StringLookup> stringLookup
* The above examples convert {@code "SGVsbG9Xb3JsZCE="} to {@code
"HelloWorld!"}.
* </p>
*
- * @return The DateStringLookup singleton instance.
+ * @return The Base64DecoderStringLookup singleton instance.
Review Comment:
The name "Base64DecoderStringLookup" does not exist as a class or type, what
is it supposed to refer to? It's confusing to me because I expect such a name
to be a class.
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -579,7 +591,7 @@ public <R, U> BiStringLookup<U>
biFunctionStringLookup(final BiFunction<String,
* The above examples convert {@code java.awt.event.KeyEvent.VK_ESCAPE} to
{@code "27"}.
* </p>
*
- * @return The DateStringLookup singleton instance.
+ * @return The ConstantStringLookup singleton instance.
Review Comment:
I do not think we should use "ConstantStringLookup" because it is a
package-private class by design and it does not help users to know the private
name, so "foo StringLookup" would be better for all of these unless the name is
public or protected.
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -629,17 +641,24 @@ public StringLookup dateStringLookup() {
*
StringLookupFactory.INSTANCE.dnsStringLookup().lookup("address|apache.org");
* </pre>
* <p>
- * Using a {@link StringSubstitutor}:
+ * When used through a {@link StringSubstitutor}, this lookup must either
be added programmatically
+ * (as below) or enabled as a default lookup using the {@value
#DEFAULT_STRING_LOOKUPS_PROPERTY} system property
+ * (see class documentation).
* </p>
*
* <pre>
- * StringSubstitutor.createInterpolator().replace("...
${dns:address|apache.org} ..."));
+ * Map<String, StringLookup> lookupMap = new HashMap<>();
+ * lookupMap.put("dns", StringLookupFactory.INSTANCE.dnsStringLookup());
+ *
+ * StringLookup variableResolver =
StringLookupFactory.INSTANCE.interpolatorStringLookup(lookupMap, null, false);
+ *
+ * new StringSubstitutor(variableResolver).replace("...
${dns:address|apache.org} ...");
* </pre>
* <p>
- * The above examples convert {@code "address|apache.org"} to {@code
"95.216.24.32} (or {@code "40.79.78.1"}).
+ * The above examples convert {@code "address|apache.org"} to the IP
address of {@code apache.org}.
* </p>
*
- * @return the DateStringLookup singleton instance.
+ * @return the DnsStringLookup singleton instance.
Review Comment:
See above, private class.
##########
src/main/java/org/apache/commons/text/lookup/DefaultStringLookup.java:
##########
@@ -31,100 +34,125 @@
public enum DefaultStringLookup {
/**
- * The lookup for Base64 decoding using the key {@value
StringLookupFactory#KEY_BASE64_DECODER}.
+ * The lookup for Base64 decoding using the key {@code "base64Decoder"}.
Review Comment:
Wrong fix here and in subsequent comments, instead, use the FQCN, for
example, replace:
```
{@value StringLookupFactory#KEY_BASE64_DECODER}
```
with
```
{@value
org.apache.commons.text.lookup.StringLookupFactory#KEY_BASE64_DECODER}
```
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
Review Comment:
This is not a "getter", so the Javadoc should not be a "Gets..." comment,
instead "to" methods convert things, so I would use "Converts..."
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -525,7 +537,7 @@ public StringLookup base64EncoderStringLookup() {
* The above examples convert {@code "SGVsbG9Xb3JsZCE="} to {@code
"HelloWorld!"}.
* </p>
*
- * @return The DateStringLookup singleton instance.
+ * @return The Base64DecoderStringLookup singleton instance.
Review Comment:
See above.
##########
src/main/java/org/apache/commons/text/StringSubstitutor.java:
##########
@@ -141,26 +141,28 @@
*
* <pre>
* final StringSubstitutor interpolator =
StringSubstitutor.createInterpolator();
- * interpolator.setEnableSubstitutionInVariables(true); // Allows for nested
$'s.
- * final String text = interpolator.replace("Base64 Decoder:
${base64Decoder:SGVsbG9Xb3JsZCE=}\n"
Review Comment:
See my other comment below for this type of comment.
##########
src/site/xdoc/userguide.xml:
##########
@@ -181,36 +181,39 @@ limitations under the License.
<section name="text.lookup">
<p>Provides algorithms for looking up strings used by a
<a
href="http://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/StringSubstitutor.html";>StringSubstitutor</a>.
- where you can select which lookup are used from
- <a
href="http://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/lookup/StringLookupFactory.html";>StringLookupFactory</a>.</p>
+ Standard lookups are defined in
+ <a
href="http://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/lookup/StringLookupFactory.html";>StringLookupFactory</a>
+ and the associated
+ <a
href="http://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/lookup/DefaultStringLookup.html";>DefaultStringLookup</a>
+ enum.
+ </p>
<p>
- The SS lets you build complex strings:
+ The example below demonstrates use of the default lookups for
<code>StringSubstitutor</code> in order to
+ construct a complex string.
+ </p>
+ <p><strong>NOTE:</strong> The list of lookups available by default
changed in version 1.10.0. See the documentation for
+ <a
href="http://commons.apache.org/proper/commons-text/apidocs/org/apache/commons/text/lookup/StringLookupFactory.html";>StringLookupFactory</a>
+ for details and instructions on how to reproduce the previous behavior.
</p>
- <code>
+ <source>
final StringSubstitutor interpolator = StringSubstitutor.createInterpolator();
-interpolator.setEnableSubstitutionInVariables(true); // Allows for nested $'s.
Review Comment:
The docs are worse IMO, not just here. We want more documentation now that
we've made the behavior more complicated, not less.
IMO the PR should do one of:
- Add inline comment at the lines showing which examples are `// not enabled
by default`
- Have 2 sections, one for the "Default Configuration", the second for the
"Full Configuration"
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -498,7 +510,7 @@ public StringLookup base64DecoderStringLookup() {
* The above examples convert {@code } to {@code "SGVsbG9Xb3JsZCE="}.
* </p>
*
- * @return The DateStringLookup singleton instance.
+ * @return The Base64EncoderStringLookup singleton instance.
Review Comment:
See above.
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
+ * @param key string to convert to a string lookup map key
+ * @return string lookup map key
+ */
+ static String toKey(final String key) {
+ return key.toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * Internal class used to construct the default {@link StringLookup} map
used by
+ * {@link StringLookupFactory#addDefaultStringLookups(Map)}.
+ */
+ static final class DefaultStringLookupsHolder {
+
+ /** Singleton instance, initialized with the system properties. */
+ static final DefaultStringLookupsHolder INSTANCE = new
DefaultStringLookupsHolder(System.getProperties());
+
+ /** Default string lookup map. */
+ private final Map<String, StringLookup> defaultStringLookups;
+
+ /**
+ * Construct a new instance initialized with the given properties.
Review Comment:
Construct -> Constructs
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
+ * @param key string to convert to a string lookup map key
+ * @return string lookup map key
+ */
+ static String toKey(final String key) {
+ return key.toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * Internal class used to construct the default {@link StringLookup} map
used by
+ * {@link StringLookupFactory#addDefaultStringLookups(Map)}.
+ */
+ static final class DefaultStringLookupsHolder {
+
+ /** Singleton instance, initialized with the system properties. */
+ static final DefaultStringLookupsHolder INSTANCE = new
DefaultStringLookupsHolder(System.getProperties());
+
+ /** Default string lookup map. */
+ private final Map<String, StringLookup> defaultStringLookups;
+
+ /**
+ * Construct a new instance initialized with the given properties.
+ * @param props initialization properties
+ */
+ DefaultStringLookupsHolder(final Properties props) {
+ final Map<String, StringLookup> lookups =
+
props.containsKey(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY)
+ ?
parseStringLookups(props.getProperty(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY))
+ : createDefaultStringLookups();
+
+ defaultStringLookups = Collections.unmodifiableMap(lookups);
+ }
+
+ /**
+ * Get the default string lookups map.
+ * @return default string lookups map
+ */
+ Map<String, StringLookup> getDefaultStringLookups() {
+ return defaultStringLookups;
+ }
+
+ /**
+ * Create the lookup map used when the user has requested no
customization.
+ * @return default lookup map
+ */
+ private static Map<String, StringLookup> createDefaultStringLookups() {
+ final Map<String, StringLookup> lookupMap = new HashMap<>();
+
+ addLookup(DefaultStringLookup.BASE64_DECODER, lookupMap);
+ addLookup(DefaultStringLookup.BASE64_ENCODER, lookupMap);
+ addLookup(DefaultStringLookup.CONST, lookupMap);
+ addLookup(DefaultStringLookup.DATE, lookupMap);
+ addLookup(DefaultStringLookup.ENVIRONMENT, lookupMap);
+ addLookup(DefaultStringLookup.FILE, lookupMap);
+ addLookup(DefaultStringLookup.JAVA, lookupMap);
+ addLookup(DefaultStringLookup.LOCAL_HOST, lookupMap);
+ addLookup(DefaultStringLookup.PROPERTIES, lookupMap);
+ addLookup(DefaultStringLookup.RESOURCE_BUNDLE, lookupMap);
+ addLookup(DefaultStringLookup.SYSTEM_PROPERTIES, lookupMap);
+ addLookup(DefaultStringLookup.URL_DECODER, lookupMap);
+ addLookup(DefaultStringLookup.URL_ENCODER, lookupMap);
+ addLookup(DefaultStringLookup.XML, lookupMap);
+
+ return lookupMap;
+ }
+
+ /**
+ * Construct a lookup map by parsing the given string. The string is
expected to contain
+ * comma or space-separated names of values from the {@link
DefaultStringLookup} enum. If
+ * the given string is null or empty, an empty map is returned.
+ * @param str string to parse; may be null or empty
+ * @return lookup map parsed from the given string
+ */
+ private static Map<String, StringLookup> parseStringLookups(final
String str) {
+ final Map<String, StringLookup> lookupMap = new HashMap<>();
+
+ try {
+ for (final String lookupName : str.split("[\\s,]+")) {
+ if (!lookupName.isEmpty()) {
+
addLookup(DefaultStringLookup.valueOf(lookupName.toUpperCase()), lookupMap);
+ }
+ }
+ } catch (IllegalArgumentException exc) {
+ throw new IllegalArgumentException("Invalid default string
lookups definition: " + str, exc);
+ }
+
+ return lookupMap;
+ }
+
+ /**
+ * Add the key and string lookup from {@code lookup} to {@code map},
also adding any additional
Review Comment:
Add -> Adds
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
+ * @param key string to convert to a string lookup map key
+ * @return string lookup map key
+ */
+ static String toKey(final String key) {
+ return key.toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * Internal class used to construct the default {@link StringLookup} map
used by
+ * {@link StringLookupFactory#addDefaultStringLookups(Map)}.
+ */
+ static final class DefaultStringLookupsHolder {
+
+ /** Singleton instance, initialized with the system properties. */
+ static final DefaultStringLookupsHolder INSTANCE = new
DefaultStringLookupsHolder(System.getProperties());
+
+ /** Default string lookup map. */
+ private final Map<String, StringLookup> defaultStringLookups;
+
+ /**
+ * Construct a new instance initialized with the given properties.
+ * @param props initialization properties
+ */
+ DefaultStringLookupsHolder(final Properties props) {
+ final Map<String, StringLookup> lookups =
+
props.containsKey(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY)
+ ?
parseStringLookups(props.getProperty(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY))
+ : createDefaultStringLookups();
+
+ defaultStringLookups = Collections.unmodifiableMap(lookups);
+ }
+
+ /**
+ * Get the default string lookups map.
Review Comment:
Get -> Gets
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
+ * @param key string to convert to a string lookup map key
+ * @return string lookup map key
+ */
+ static String toKey(final String key) {
+ return key.toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * Internal class used to construct the default {@link StringLookup} map
used by
+ * {@link StringLookupFactory#addDefaultStringLookups(Map)}.
+ */
+ static final class DefaultStringLookupsHolder {
+
+ /** Singleton instance, initialized with the system properties. */
+ static final DefaultStringLookupsHolder INSTANCE = new
DefaultStringLookupsHolder(System.getProperties());
+
+ /** Default string lookup map. */
+ private final Map<String, StringLookup> defaultStringLookups;
+
+ /**
+ * Construct a new instance initialized with the given properties.
+ * @param props initialization properties
+ */
+ DefaultStringLookupsHolder(final Properties props) {
+ final Map<String, StringLookup> lookups =
+
props.containsKey(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY)
+ ?
parseStringLookups(props.getProperty(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY))
+ : createDefaultStringLookups();
+
+ defaultStringLookups = Collections.unmodifiableMap(lookups);
+ }
+
+ /**
+ * Get the default string lookups map.
+ * @return default string lookups map
+ */
+ Map<String, StringLookup> getDefaultStringLookups() {
+ return defaultStringLookups;
+ }
+
+ /**
+ * Create the lookup map used when the user has requested no
customization.
Review Comment:
Create -> Creates
##########
src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java:
##########
@@ -1171,4 +1211,109 @@ public StringLookup xmlStringLookup() {
return XmlStringLookup.INSTANCE;
}
+ /**
+ * Get a string suitable for use as a key in the string lookup map.
+ * @param key string to convert to a string lookup map key
+ * @return string lookup map key
+ */
+ static String toKey(final String key) {
+ return key.toLowerCase(Locale.ROOT);
+ }
+
+ /**
+ * Internal class used to construct the default {@link StringLookup} map
used by
+ * {@link StringLookupFactory#addDefaultStringLookups(Map)}.
+ */
+ static final class DefaultStringLookupsHolder {
+
+ /** Singleton instance, initialized with the system properties. */
+ static final DefaultStringLookupsHolder INSTANCE = new
DefaultStringLookupsHolder(System.getProperties());
+
+ /** Default string lookup map. */
+ private final Map<String, StringLookup> defaultStringLookups;
+
+ /**
+ * Construct a new instance initialized with the given properties.
+ * @param props initialization properties
+ */
+ DefaultStringLookupsHolder(final Properties props) {
+ final Map<String, StringLookup> lookups =
+
props.containsKey(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY)
+ ?
parseStringLookups(props.getProperty(StringLookupFactory.DEFAULT_STRING_LOOKUPS_PROPERTY))
+ : createDefaultStringLookups();
+
+ defaultStringLookups = Collections.unmodifiableMap(lookups);
+ }
+
+ /**
+ * Get the default string lookups map.
+ * @return default string lookups map
+ */
+ Map<String, StringLookup> getDefaultStringLookups() {
+ return defaultStringLookups;
+ }
+
+ /**
+ * Create the lookup map used when the user has requested no
customization.
+ * @return default lookup map
+ */
+ private static Map<String, StringLookup> createDefaultStringLookups() {
+ final Map<String, StringLookup> lookupMap = new HashMap<>();
+
+ addLookup(DefaultStringLookup.BASE64_DECODER, lookupMap);
+ addLookup(DefaultStringLookup.BASE64_ENCODER, lookupMap);
+ addLookup(DefaultStringLookup.CONST, lookupMap);
+ addLookup(DefaultStringLookup.DATE, lookupMap);
+ addLookup(DefaultStringLookup.ENVIRONMENT, lookupMap);
+ addLookup(DefaultStringLookup.FILE, lookupMap);
+ addLookup(DefaultStringLookup.JAVA, lookupMap);
+ addLookup(DefaultStringLookup.LOCAL_HOST, lookupMap);
+ addLookup(DefaultStringLookup.PROPERTIES, lookupMap);
+ addLookup(DefaultStringLookup.RESOURCE_BUNDLE, lookupMap);
+ addLookup(DefaultStringLookup.SYSTEM_PROPERTIES, lookupMap);
+ addLookup(DefaultStringLookup.URL_DECODER, lookupMap);
+ addLookup(DefaultStringLookup.URL_ENCODER, lookupMap);
+ addLookup(DefaultStringLookup.XML, lookupMap);
+
+ return lookupMap;
+ }
+
+ /**
+ * Construct a lookup map by parsing the given string. The string is
expected to contain
Review Comment:
I usually like to reserve the use of "Constructs" comments to constructors,
this method looks like it "Parses.... and returns a Map".
##########
src/main/java/org/apache/commons/text/StringSubstitutor.java:
##########
@@ -292,9 +294,81 @@ public String toString() {
*
* <pre>
* StringSubstitutor.createInterpolator().replace(
- * "OS name: ${sys:os.name}, " + "3 + 4 = ${script:javascript:3 + 4}");
+ * "OS name: ${sys:os.name}, user: ${env:USER}");
* </pre>
*
+ * <p>The table below lists the lookups available by default in the
returned instance. These
+ * may be modified through the use of the {@value
StringLookupFactory#DEFAULT_STRING_LOOKUPS_PROPERTY}
+ * system property, as described in the {@link StringLookupFactory}
documentation.</p>
+ *
+ * <p><strong>NOTE:</strong> The list of lookups available by default
changed in version {@code 1.10.0}.
+ * Configuration via system property (as mentioned above) may be necessary
to reproduce previous functionality.
Review Comment:
"may be"? You MUST do something if you want the previous behavior. I would
be more explicit and list the lookups removed from the default here (DNS, ...)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
