Can you turn that concern into a more firm stance? FWIW if you look closely, there's really only about 100 lines of greenfield code in /r/42042 (CommandLineParameters.java, sure to grow of course). There's a lot of bulk due to splitting out parser classes; commons-args has the same.
I should have been clear - i'm not married to this. Mostly scratching an old itch, i won't be upset if it's more churn than it's worth at this time. (I _am_ tired of the gradle and intellij issues though.) On Thu, Jan 7, 2016 at 6:31 PM, Maxim Khutornenko <ma...@apache.org> wrote: > I am a big +1 to the refactoring if only for testability reasons. > > My only concern is the amount of supporting code that your POC > resulted in https://reviews.apache.org/r/42042. It may feel like > trading code with Commons parser at the end, which is battle proven > and well tested. That said, arg parsing is generally a well scoped > problem, so it should be relatively easy to cover major testing > scenarios. > > On Thu, Jan 7, 2016 at 5:18 PM, Bill Farner <wfar...@apache.org> wrote: > > All, > > > > I propose that we replace our current command line args system. For > those > > unaware, the args system [1] we currently use was forked out of Twitter > > Commons several months ago. The goal when forking was to adapt/maintain > or > > eventually replace these libraries. Given the code volume and complexity > > of commons-args, i propose we replace it and address some issues along > the > > way. > > > > I suggest several goals: > > a. sidestep current development hurdles we have encountered with the args > > system (intellij/gradle not working nicely with apt) > > b. encourage better testability of Module classes by always injecting all > > args > > c. leverage a well-maintained third-party argument parsing library > > d. stretch: enable user-friendly features like logical option groups for > > better help/usage output > > e. stretch: enable alternative configuration inputs like a configuration > > file or environment variables > > > > (b) is currently an issue because command line arguments are driven from > > pseudo-constants within the code, for example: > > > > @NotNull > > @CmdLine(name = "cluster_name", help = "Name to identify the cluster > > being served.") > > private static final Arg<String> CLUSTER_NAME = Arg.create(); > > > > @NotNull > > @NotEmpty > > @CmdLine(name = "serverset_path", help = "ZooKeeper ServerSet path to > > register at.") > > private static final Arg<String> SERVERSET_PATH = Arg.create(); > > > > This makes it simple to add command line arguments. However, it means > that > > a level of indirection is needed to parameterize and test the code > > consuming arg values. We have various examples of this throughout the > > project. > > > > I propose that we change the way command line arguments are declared such > > that a Module with the above arguments would instead declare an interface > > that produces its parameters: > > > > public interface Params { > > @Help("Name to identify the cluster being served.") > > String clusterName(); > > > > @NotEmpty > > @Help("ZooKeeper ServerSet path to register at.") > > String serversetPath(); > > } > > > > public SchedulerModule(Params params) { > > // Params are supplied to the module constructor. > > } > > > > Please see this review for a complete example of this part of the change: > > https://reviews.apache.org/r/41804 > > > > This is roughly the same amount of overhead for declaring arguments as > the > > current scenario, with the addition of a very obvious mechanism for > > swapping the source of parameters. This allows us to isolate the body of > > code responsible for supplying configuration values, which we lack today. > > > > The remaining work is to bridge the gap between a command line argument > > system and Parameter interfaces. This is relatively easy to do with > > dynamic proxies. I have posted a proof of concept here: > > https://reviews.apache.org/r/42042 > > > > Regarding (c), i have done some analysis of libraries available and i > > suggest argparse4j [3]. It has thorough documentation, no transitive > > dependencies, and is being actively developed (last release Dec 2015). > > However, i would like to emphasize that i think we should minimize > coupling > > to the argument parsing library so that we may switch in the future. > > Argparse4j has a feature that makes the non-critical feature (d) > possible. > > > > With that, what do you think? Are there other goals we should add? Does > > the plan make sense? > > > > [1] > > > https://github.com/apache/aurora/tree/master/commons-args/src/main/java/org/apache/aurora/common/args > > [2] https://github.com/twitter/commons > > [3] https://argparse4j.github.io/ >
