On 02/16/2013 03:04 PM, Duncan Jones wrote: > Hi, > > The Option class has some odd characteristics that I think should be > addressed: > > 1. The class can be constructed with null values, either by using the > default builder constructor (discussed in CLI-224) or by passing null > values into the constructor. I think instead we should define the > minimum information required for a meaningful Option (short option and > description?) and enforce that the user passes these. > > Without such a change, it's possible to produce nonsense options and > cause exceptions, e.g. > > Option option = new Option(null, null); > option.getId(); // throws NullPointerException > > > In a similar vein, I think the new default constructor proposed for > the builder in CLI-224 should go.
I am fine to add additional sanity checks for nonsense input. > 2. The equals method only inspects the short and the long option. Even > if this is a good idea (and I'm quite convinced it isn't), it should > at least be documented. Otherwise, one can produce surprisingly > "equal" objects, such as: > > Option option = new Option(null, "foo"); > Option option2 = new Option(null, "bar"); > assertNotEquals(option, option2); // Fails because objects are "equal" > > Unless there is a good reason, the equals method should test all fields. > > Any thoughts on this? Can we make such changes without breaking too > much backwards compatibility? The parser takes advantage of this wrt required options in some weird way which I honestly do not want to touch as it seems to work. The 1.x API has several flaws, and I am not sure if it is worth correcting them. My intention is to do a 1.3 release with several bugfixes / new features (like the new DefaultParser) and then move on to cli 2 which will hopefully fix these problems. Thomas --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
