[WIP] for #30 object type dispatch

timotheecour commented 5 years ago

@c-blake help welcome! this is a WIP towards getting some proof of concept for #30 ...

btw: I won't force push to this branch so feel free to contribute to that pr_fix_30 branch (eg via PR's to my fork); might be easiest way to collaborate on that feature (unless you have other ideas)

proposal

let's focus on getting something useful working first (no need to cover all cases in the beginning)
when there's a single param, and it's type is object or ref object or tuple, treat it as if it were multiple fields, all of which being optional see added test case test/t0021.nim
the autogenerated comments can come later

c-blake commented 5 years ago

I think your starting with a limited ambitions is the right way to go...While our impulse may be to get recursively complete everything working, most of the value is probably in some very simple cases like the one level of fields with no name collisions. There are real hierarchy collapse collision complexities both for the impl and CLI author (and maybe even CLI user) to worry about. It's also dirt simple to run into all sorts of macro-time issues with Nim and have to go through trial and error to find workarounds.

One of my "meta strategies", if it is not just obvious, is to try to offload as much stuff as possible into the generated code and to "later stages" of Nim processing. Later==more "filled in"/less buggy. Sometimes that requires "rotating the problem" a bit. E.g., argParse used to be a per-type template, but generic overloading was broken for templates (I think it's since been fixed) which made enum handling impossible. So, I changed argParse to just a proc taking a var arg. That was a really breaking change for anyone using beyond the for-dummies mode. Still, looking at your argParse makes me happy about that move. I'd be unsurprised if those specific overloads with more complex type predicates still failed for templates.

Hopefully we can make some progress on this without something invasive to users who do not use the feature, but that's not a 100% requirement if you can find a workaround which gets it working and maybe seems like it could help in other situations. I'm liking how little extra code I see to handle this so far. I don't have specific ideas on working around your Nim issue, but maybe some one over @nim-core will.

Also, I'm inclined to just release 0.9.18 in the next couple days without any more new features. Also, you seem empirically more interested in other things than https://github.com/c-blake/cligen/issues/48 which you never really even discussed back.

c-blake commented 5 years ago

I think this is resolved by https://github.com/c-blake/cligen/commit/07e0f319adf13ce3eca3b69b0acf6d0e63de5812 which takes a somewhat different approach (in brief restricting to just top-level fields needing argParse and generating a full initializer instead of trying to argParse an object) as foreshadowed by various comments in https://github.com/c-blake/cligen/issues/30.

Anyway, the CLI author visible API is arguably even simpler. E.g. this can work:

type App = ... # definition
var app = initFromCL(App()) #; app logic now...

Tuples also supported. See test/InitOb.nim and test/InitTup.nim.

c-blake / cligen

[WIP] for #30 object type dispatch #80

proposal