"Can you turn that concern into a more firm stance?" - you mastered the original library and I fully trust you with the new one :)
+1. On Thu, Jan 7, 2016 at 6:58 PM, Bill Farner <wfar...@apache.org> wrote: > 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/ >>
