c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
496 stars 23 forks source link

Check that bounded integrals’ values belong to their range #187

Closed SirNickolas closed 3 years ago

SirNickolas commented 3 years ago

Thank you for clarification. I did not realize that all that was needed was to simply write a definition for argParse & argHelp. Here they are.

c-blake commented 3 years ago

That's the main architecture of cligen, really. Could almost use $ instead of argHelp, but "how things are formatted" may be specific to types. If everyone always used parseMyType like initType that could almost be used as well, but for possibly command-line specific parsing syntax. Because of those two irregularities, I figured I should just do argHelp/argParse. You should always be able to do a distinct type to get what you want, but these changes look pretty ok to me. Will test them out in the AM.

SirNickolas commented 3 years ago

And if you want you could say 0.. instead of Positive and 1.. instead of Natural

Did that. I omit lower or upper boundary if it exceeds one billion (it is large enough to not care about a particular value).

c-blake commented 3 years ago

For older versions of Nim (0.19.2, 0.20.2, etc.) I get errors compiling test/IntRanges.nim. Earlier ones get ambiguous $ calls { system.$(x: uint64) | system.$(x: int) } just compiling test/IntRanges.nim. Later Nims, like 0.20.2, get cligen/argcvt.nim(212, 16): type mismatch.

I'm not sure that really matters, but I thought I should mention it. I try to keep all the basic stuff working for >= 0.19.2, but some stuff just can't and if it's just one or two programs with isolated latterday cligen features then that's no big deal and here it only impacts compiling test/IntRanges.nim. It would be a bigger problem if your refactoring broke int|float (almost surely used types) on old setups, but I don't think it does. At least test/AllTypes works on 0.19.2.

( You think you had an expensive test.sh experience..I do for nim in /.../nim-*/bin/nim nim; { for BE in 'c' 'cpp --cc:gcc'; { ncl; echo "Nim Version Is $nim; BE is $BE"; nim="$nim" BE="$BE" ./test.sh } }, though usually only as a new release approaches. )

Anyway, this is looking pretty good to me. Let me know when you think it's ready to merge and I will.

c-blake commented 3 years ago

I realize that 0.19.2 is a 2.5 year old release, but many Linux distros lag behind more years than that and also people come & go from the Nim community. So, if it's not hard, I try not to break old stuff. Others have asked, e.g. https://github.com/c-blake/cligen/issues/85. { Maybe @genotrance can thank me by someday having nimble init not create src/ by default, a poor organization 90+% of the time. ;-) }

SirNickolas commented 3 years ago

Oops. Downloaded 0.19.6 and 0.20.2 and checked with them.

Also, extended X..Y syntax to float range types as well. But there is a problem in Nim 0.19: high and low for range[SomeFloat] there return Inf and NegInf, respectively, instead of correct values for that range. They became magic in 0.20 so it is unlikely we are able to do something with them. (I tried to declare my own magic functions, but they didn’t work. Seems I’m not a wizard, sadly.)

c-blake commented 3 years ago

I think it's fine if all this range stuff only works post Nim-0.20.2 or so. It may make sense to advertise that as the new minimum version soon.

I would say, though, that you should rename test/IntRanges.nim to test/NumberRanges.nim or test/RangeTypes.nim and add some float range tests. (You could add a whole other program, but it's better to keep the test suite small if everything conceptually fits in one program.)

SirNickolas commented 3 years ago

I decided not to add float there so that the test is runnable under 0.19. But if you are planning to drop its support eventually, I’ll do it.

c-blake commented 3 years ago

I am planning to drop 0.19 support eventually..maybe after nim-1.6 has stabilized in a few months.

If you want you can put various things in the code and test behind some when (NimMajor,NimMinor,NimPatch) >= (0,20,2) guard or something.

Or we could just not do float ranges for a while. You're the first person to ask for even integer ranges in over 5 years. I'm fine either way.

SirNickolas commented 3 years ago

I think, it is good enough now. Could you look, please?