Hi Mandy,
On 23/09/15 01:02, Mandy Chung wrote:
On 09/22/2015 09:27 AM, Daniel Fuchs wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.02
Looking quite good. Thanks for making the change.
Below are mostly javadoc comments and some minor implementation comments.
1474 * which should be in {@linkplain java.util.Properties
properties file} format.
Would it be better to state this above in @throws IOException:
IOException if there are problems reading from the stream or
the given stream is not in Properties format
Done.
1484 * existing loggers that have a level property specified in the
new
1485 * configuration stream
s/the new configuration stream/the given input stream/
1491 * methods instead.
s/methods/method/
Done.
updateConfiguration(Function) - slight reorg of the sentences:
* Updates the logging configuration.
* <p>
* If the "java.util.logging.config.file" system property is set,
* then the property value specifies the properties file to be read
* as the new configuration. Otherwise, the LogManager default
* configuration is used.
* The default configuration is typically loaded from the
* properties file "{@code conf/logging.properties}" in the Java
installation
* directory.
* This method reads the new configuration and calls the {@link
* #updateConfiguration(java.io.InputStream,
java.util.function.Function)
* updateConfiguration(ins, mapper)} method to
* update the configuration.
*
* @apiNote
* This method updates the logging configuration from reading
* a properties file and ignores the "java.util.logging.config.class"
* system property. The "java.util.logging.config.class" property is
* only used by the readConfiguration method to load a custom
configuration
* class as an initial configuration.
1774 * For each configuration key <i>k</i>,
Could simply be "For each {@code k},"
Done.
- do you want italic for all occurrance? Maybe just the first occurrance
and @code for the subsequent ones.
Mixing italics and {@code } was a bit confusing to me. When I generated
the javadoc and looked at the specdiff, it looked as if {@code k} was a
different thing than the <i>k</i> we had discussed earlier.
So I choose to use italics for formal concepts/hypothetical variables
and {@code } for actual code and @param names.
1780 * A {@code null} value for <i>v</i> indicates that there
should be no
1781 * value associated with <i>k</i> in the resulting
configuration.
This sentence can be combined with line 1772-1773 as follows:
returns a function <i>f(o,n)</i> whose returnedvalue will be
applied to the resulting configuration, or {@code null}
to indicate that the property {@code k} will not be added
in the resulting configuration.
OK - but it would be better to break the sentence:
returns a function <i>f(o,n)</i> whose returned
value will be applied to the resulting configuration.
The function <i>f</i> may return {@code null} to indicate
that the property <i>k</i> should not be added to the
resulting configuration.
Also @param mapper - need to say "mapper may be null"
Same comment applies to @param in the other updateConfiguration method.
right.
1825 * then <i>v = f(o,n)</i> is the
1826 * resulting value (i.e. the value that will be associated with
1827 * <i>k</i> in the resulting configuration).
Might be something like this:
then <i>v = f(o,n)</i> is the returned value.
If v is not null, the property k with value v will be added in
the resulting configuration. Otherwise, it will not be added.
then <i>v = f(o,n)</i> is the resulting value.
If <i>v</i> is not {@code null}, then a property
<i>k</i> with value <i>v</i> will be added to the resulting
configuration. Otherwise, it will be omitted.
1842 * listener}
Should it be "listener(s)"
OK.
2018 // Builds a TreeSet of all (old and new) property names.
2030 .forEachOrdered(k -> ConfigProperty
Is the ordering matter here?
Not really. It does matter later - we want to handle
parent loggers before child loggers to optimize the
processing but we use a TreeMap for that.
TreeSet is probably a remnant from an earlier prototype.
2071 if (l == null) continue;
2072 if (!visited.test(l)) {
Nit: can be combined in one line
if (l != null && !visited.test(l)) {
Done.
ah - I saw that is used in line 2166
New tests: I only skimmed through them. They are good and covers complex
scenarios. For new methods like this, I generally like to see simple
and small unit tests to be added that are easy for readers to
understand. For example, some sample properties files and check the
resulting configuration using LogManager.getProperty. You already have
many test cases covered. I didn't want to add more works but it'd be
really nice to see simple small unit tests. Maybe refactor
UpdateConfigurationTest.
OK - I will add a simple test. I think I'll probably use the
mapper examples from the documentation as my test cases :-)
I'll send a new webrev when ready.
-- daniel
Mandy
