Re: [CLI] Option class may need some attention
On 02/16/2013 10:18 PM, Duncan Jones wrote: On 16 February 2013 20:53, Thomas Neidhart thomas.neidh...@gmail.com wrote: 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. OK. I would suggest the short option is the only required parameter (non-null, non-empty) - a description should be optional. What do you say? You either have to specify a short or long option (or both of course). The old OptionBuilder actually checks this condition in the create method. When creating an Option yourself, this can not be checked in the constructor, as you may call setLongOption afterwards. This should be added to the javadoc and the new Option.Builder should be adapted accordingly. There is no need to create a separate issue for this, I will do the necessary changes. Thomas - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[CLI] Option class may need some attention
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. 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? Duncan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [CLI] Option class may need some attention
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
Re: [CLI] Option class may need some attention
On 16 February 2013 20:53, Thomas Neidhart thomas.neidh...@gmail.com wrote: 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. OK. I would suggest the short option is the only required parameter (non-null, non-empty) - a description should be optional. What do you say? 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. Ok, fair enough. Last year I was hunting for someone interested in pursuing CLI 2.x, so let me know when you turn your attention there and I'll help out. Duncan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
