[FEATURE] Refector the inner classfor Setting

[mloufra]

Is your feature request related to a problem?

As this comment mentioned. There are too many parser class which implement Function<Setting, ?> and Writeable was added in Setting.java. Here is the one of the Writeable Parser class public static class FloatParser implements Function<String, Float>, Writeable in the Setting class

What solution would you like?

Create a separate class for these writeable parsers and store them in one place instead of Setting.java class

[owaiskazi19]

Can you add more details in this issue @mloufra? Add a link of the comment from the PR and you can also link the file here

[mloufra]

Can you add more details in this issue @mloufra? Add a link of the comment from the PR and you can also link the file here

sure

[dbwiddis]

Good discussion to have and I'll toss in a link to a comment by @reta when WriteableSetting was merged.

TLDR: We can refactor the existing code but most of it still needs to exist, it's just a question of how to reorganize the logic and where to put it, and I somewhat prefer the existing implementation @mloufra is doing to address #349.

Extended background:

1. The Setting<T> class is not Writeable and includes up to three non-Writable arguments in its public constructors. See for example this constructor, specifically the default value, parser, and validator:

   • Settings are internally stored as a string (writeable)
   • Default value can refer to another setting (potentially a long daisy chain) that all ends at a string
   • Parser turns the String into a type T, each of which has a different return value, and necessitates either returning the specific type (multiple parsers) or having multiple methods, each of which returns the specific type and the others of which are invalid, or returning a more general type (e.g., java.lang.Number that still has type-specific getters (e.g., intValue(), longValue(), etc.)
   • Parsers also often perform numeric validation (min/max value)
   • Validator performs other validation, primarily for things the parser can't (e.g., String validation).

2. Ideally we would want to update the javadoc to specify that T should be writeable and handle it. We're still going to have to have a Parser that handles it and returns its value.

   • I currently favor the existing implementation because users can use the generic constructor we have and either write their own parser (e.g., BigIntegerParser() or maybe IPAddressParser, DegreesMinutesSecondsParser, etc.) using the existing parsers as a pattern, or override/extend the implementations @mloufra made.
     • Combining everything in a single Parser class won't permit these, and BigInteger or BigDecimal are reasonable settings to move beyond 64 bit numbers
     • One of my earliest open source contributions involved extending JPPF to prioritize traffic to nodes based on netmasks parsing CIDR notation (IP address + integer) which were read in from a settings file, so that seems like a reasonable capability to permit; if we don't code it we must at least allow the flexibility for a user to do so
     • OpenSearch provides Geographic location support, we currently don't have settings for that but could store latitude/longitude etc. Currently we'd have to hack together a decimal number and character (+/- 90.0 N/S or +/- 180.0 E/W) to model that
   • We could certainly move the parsers to a different class/classes but they currently leverage some private methods inside Setting that would need to be moved or made public to support.

3. The initial work I did in #4519 (feature branch, eventually merged into main in #5597) was the minimum necessary to get all the Setting usages in the AD plugin minimally working by transferring over the original/default value and type (which included the non-restricted parser function).

   • Unfortunately this lost reference to the original parsing functions (e.g., non-negative numeric values). In an attempt not to edit the actual Setting class, I created a new WriteableSetting class that attempted to centralize all the switch/case logic of handling the various types.
   • A lot of that logic could be moved inside each specific writeable parser class, and that seems like a reasonable thing to do, but we're still going to need a different parser class for each T we handle.

So, if I were refactoring it, I'd probably make new standalone classes for each of the different T classes we currently handle (Boolean, Float, Double, Byte, Short, Int, Long, TimeValue, ByteSizeValue) which are mostly the existing inner classes, but move in some of the (currently private) parsing functions from Setting and the writeTo/streaminput constructor logic from WriteableSetting, such that WriteableSetting can go away, Setting will be smaller, and we'll still keep the current functionality.

[owaiskazi19]

Thanks for the detailed explanation @dbwiddis! I am inline with creating different T classes for the parsers to make Setting smaller and making the parsers more modular. I would label this issue as a good to have as moving private methods from Setting and making WriteableSetting go away can be done in future.