Interpolation Defaults #341
Conversation
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
============================================
+ Coverage 96.44% 96.52% +0.08%
+ Complexity 2338 2336 -2
============================================
Files 84 84
Lines 5821 5845 +24
Branches 957 961 +4
============================================
+ Hits 5614 5642 +28
+ Misses 128 124 -4
Partials 79 79
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -117,14 +115,6 @@ public enum DefaultStringLookup { | |||
| */ | |||
| XML(StringLookupFactory.KEY_XML, StringLookupFactory.INSTANCE.xmlStringLookup()); | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This is handy, why remote it?
There was a problem hiding this comment.
I don't believe it is appropriate to include a main method here. If the values are not easily accessible via the documentation, then that indicates a failure in the documentation and not a need for a one-off tool.
darkma773r
force-pushed
the
interpolation-defaults
branch
from
bcea4a2
to
5f168cf
Compare
|
It's good that the changes are similar to Commons Configuration but we should preserve the examples in Javadocs and perhaps separate them in a separate section in the Javadoc with an introduction sentence saying what you need to do to enable the relevant lookups. |
…dns, url, and script lookups from defaults.
darkma773r
force-pushed
the
interpolation-defaults
branch
from
5f168cf
to
1c1e138
Compare
|
@garydgregory, I've added some additional documentation. Let me know what you think. |
|
@garydgregory, any objections to merging this? |
There was a problem hiding this comment.
Hi @darkma773r
Thank you for asking me to review the PR. I am out of this morning but I've scattered some comments throughout that I hope will prove useful.
src/changes/changes.xml
Outdated
| @@ -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> | |||
There was a problem hiding this comment.
Move this to the FIX section IMO
| final StringSubstitutor interpolator = StringSubstitutor.createInterpolator(); | ||
| interpolator.setEnableSubstitutionInVariables(true); // Allows for nested $'s. |
There was a problem hiding this 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"
| @@ -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" | |||
There was a problem hiding this comment.
See my other comment below for this type of comment.
| @@ -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. | |||
There was a problem hiding this 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.
| @@ -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. | |||
There was a problem hiding this 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.
| } | ||
|
|
||
| /** | ||
| * Create the lookup map used when the user has requested no customization. |
| } | ||
|
|
||
| /** | ||
| * Construct a lookup map by parsing the given string. The string is expected to contain |
There was a problem hiding this comment.
I usually like to reserve the use of "Constructs" comments to constructors, this method looks like it "Parses.... and returns a Map".
| } | ||
|
|
||
| /** | ||
| * Add the key and string lookup from {@code lookup} to {@code map}, also adding any additional |
| * 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. |
There was a problem hiding this 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, ...)
| @@ -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"}. | |||
There was a problem hiding this 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}
Changes similar to what was recently released in commons-configuration 2.8.0.