janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
122 stars 35 forks source link

in argparse, a cfunction cannot be used for :map #157

Closed jmckernon closed 1 year ago

jmckernon commented 1 year ago
(use spork/argparse)
(pp (argparse nil "foo" {:kind :option :map |(scan-number $)})) # returns 3
(pp (argparse nil "foo" {:kind :option :map scan-number})) # returns "3"
pepe commented 1 year ago

It is correct, as per https://github.com/janet-lang/spork/blob/master/spork/argparse.janet#L127. I do not see any issue to change this behavior.

jmckernon commented 1 year ago

In my example, is it somehow better apply |(scan-number $) than scan-number? I don't see why. I don't have to care whether a function is a cfunction or not when I use one elsewhere; why should I here?

sogaiu commented 1 year ago

I went looking in the original repository for argparse and in the first commit (function? ...) is used (a slightly different location).

I think the current code under discussion was added by @tionis -- and it seems reasonable to suppose that the earlier code was emulated (perhaps he remembers).

Perhaps this was just an oversight in the original formulation -- may be the intent was actually something like:

(or (function? ...) (cfunction? ...)

and the pattern was just replicated?

jmckernon commented 1 year ago

Thankyou for looking into it.

I can't speak for the original intention, but (or (function? ...) (cfunction? ...) is closer to the behaviour I would have expected.

tionis commented 1 year ago

I just didn't think to check for cfunctions when adding :map

tionis commented 1 year ago

I added a pull request that should fix it (just added the check as @sogaiu suggested)

jmckernon commented 1 year ago

Happy with that. Thanks to you all for your help.