akamensky / argparse

Argparse for golang. Just because `flag` sucks
MIT License
604 stars 62 forks source link

shorthand argument creation check #54

Closed goofinator closed 4 years ago

goofinator commented 4 years ago

If i do some thing like this ("pp" on shorthand argument ): parser.Int("pp","port",&argparse.Options{Required: false,Default: 10000, Help: "Work in dummy mode"}) there is no error on parser.Parse, and default value became 0. Are long name as shorthand allowed? If no, maybe it should be error value on this situation? If Yes - why default value differs from expectation.

akamensky commented 4 years ago

No, Not sure how this got broken, there was (and still must be) a check that shorthand argument is one character long. (err if 0 > len(sn) > 1 because it can be 0 too).

goofinator commented 4 years ago

Ooh... I've got it. If you create an argument with wrong description, like in my case - long name for a shorthand argument, library refuses this argument's description silently. Later, when you try to use it, there will be 'unknown arguments' error. This is OK. But in the case of Options with Required: false and some Default value, it will be another default value on Parse return (default value of the requested type). That's because of pointer's creation and returning independently of o.addArg execution status. So, in case of using of Option Required: false and some error on o.addArg you will get unexpected value (default for type instead of Default by options) without any alarm. It's not good. I see next variants:

  1. build the complete error chain from addArg to user (in this case user will have to check error after each argument creation).
  2. in case of o.addArg failure - return nil pointer, so the user's program will panic on attemption to use it
  3. panic on o.addArg failure (this may take sance, because this is a case of error in user programm).
  4. or just let it go))

I like 2 and 3, but Interested in your opinion.

akamensky commented 4 years ago

To be honest I don't like 2 because of nil pointer, perhaps that is the legacy of hating nullptr from C/C++. I'd argue that if method promises to return a specific type it must try its best to return that instead of nil (except errors I guess).

I would prefer to separate a developer (who would use this library in their application) and actual application user. Letting application crash with null pointer dereference exception while user runs application is not nice. User may have no knowledge of application internals and may have 0 idea why it suddenly failed.

Instead I would prefer to have a meaningful error message returned to the developer (or user if developer did not handle this) and have it contain a message of why it failed.

If it is a developer mistake -- they should be able to catch this during their testing. If a user mistake they should see a clear explanation to their mistake.

Back to this case -- I think this library should do its best effort to get a correct input. If shorthand handle for an argument should be limited to a single character -- this must be done by the means inside library in a clear way. For example it could panic inside NewInt when shorthand is longer than 1 character (before pointer is even returned). Or it could simply accept a byte in string format. Or some other way.

akamensky commented 4 years ago

I think that was done in #57

feel free to close if this is resolved