search

DataONEorg / d1_common_java

The d1_common_java library migrated from subversion
Other
0 stars 1 forks source link

Inaccurate description of Settings properties in javadoc #1

Open srstsavage opened 3 years ago

srstsavage commented 3 years ago

Hi all, there appears to be some inaccuracies in the javadoc for the getConfiguration method of Settings.

From the javadoc:

     * System properties are always loaded first, followed by those
     * properties in an optional file (specified with the system
     * property "opt.overriding.properties.filename"), followed by
     * context-specific default properties files.

From what I can tell from testing and checking the code:

opt.overriding.properties.filename is mentioned in a SettingsTest test, but that test isn't run (the @Test annotation is commented out).

mbjones commented 3 years ago

After a quick peek, you seem to be right. Looks like one of the devs removed functionality without updating the docs, and got tests to pass by just commenting out the test (ug). So, question is, what's our move forward? Is this critical functionality that we should reintroduce, or do we just fix the docs and work with the other mechanisms for changing Settings? Thoughts?

srstsavage commented 3 years ago

IMO the current functionality is fine and the javadocs just need to be cleaned up. I spent a while trying to figure out why system properties weren't affecting the timeout. I'm guessing that others aren't using system properties or the override file either since they wouldn't be working, or they'd be using an old version of the client and could fix their settings when upgrading. So yeah, seems to me that the javadocs just need to be cleaned up.