-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interpolation Defaults #341
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is handy, why remote it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create -> Creates
} | ||
|
||
/** | ||
* Construct a lookup map by parsing the given string. The string is expected to contain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add -> Adds
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.