pacak / bpaf

Command line parser with applicative interface
337 stars 17 forks source link

Added anywhere support for derive #348

Closed vallentin closed 5 months ago

vallentin commented 5 months ago

I got curious, and wanted to add support for anywhere for derive after https://github.com/pacak/bpaf/discussions/347.

Overall I just grepped for keywords to know where I needed to add it. So feel free to nit, if you want any changes.

Additionally, I tested that it works locally, but I didn't add any tests to assert it. I'll happily add some tests, just mention where you prefer I add them.

pacak commented 5 months ago

I started implementing it by writing a test but ran out of time before having do to my $dayjob.

Can you add this at the end of bpaf_derive/src/field_tests.rs?

#[test]
fn any_anywhere() {
    let input: NamedField = parse_quote! {
        #[bpaf(any::<isize>("LIMIT", isize_to_usize), anywhere)]
        num: isize,
    };
    let output = quote! {
        ::bpaf::any::<isize, _, _>("LIMIT", isize_to_usize).anywhere()
    };
    assert_eq!(input.to_token_stream().to_string(), output.to_string());
}
vallentin commented 5 months ago

Added the test!

On an unrelated note. It's probably purely coincidence, but your response time is crazy fast

vallentin commented 5 months ago

Oh, the test failed locally. Maybe I should have run it before pushing...

pacak commented 5 months ago

Oh, the test failed locally. Maybe I should have run it before pushing...

Dropping this comma should do the trick I think.

        num: isize,   
vallentin commented 5 months ago

Yes, now it passes! I'm a bit rusty when it comes to procedural macros

pacak commented 5 months ago

Okay, now it parses. Thank you for your contribution :heart: :) I really should collect all the recent changes and release a new version.

vallentin commented 5 months ago

Just a tiny change, because I got curious. But you're welcome :)

Thank you for bpaf. 2 weeks ago I was looking into reworking CLIs in various (private) projects. Refactoring some very old structopt and clap code. After testing all major libraries, bpaf won based on features and usability! So thank you for an awesome library! :)

When I have time available, I might run through your issue list, and see if there's something I can resolve or comment on

pacak commented 5 months ago

When I have time available, I might run through your issue list, and see if there's something I can resolve or comment on

I believe my biggest problem right now is the documentation as proven by https://github.com/pacak/bpaf/discussions/347. You can do a lot of things and there's only a small handful names you need to know, but it's not always obvious what goes where. I have a bigger documentation rework in progress but it got stuck on rustdoc not being able to tell Foo<Bar>::help and Foo<NotBar>::help apart...

vallentin commented 5 months ago

I definitely agree with that. The hardest part of learning bpaf, was definitely locating infomation (and I did find the special documentations module). Overall, I usually look through examples to when it comes to derive based libraries.

Throughout the last 2 weeks, reworking the documentation was definitely on my mind.

So, now that you mention it. In Askama we have the Askama Book which uses mdbook. It's definitely nice, to be able to refer people to the book. Now, obviously it would require a significant amount of work. But I'm open to mirroring something similar for bpaf. Unless you have other ideas?

pacak commented 5 months ago

Unless you have other ideas?

Sort of. What I like about current documentation is that for most of the functions I have combinatoric and derive examples as well as expected output on a few samples: see here for example. Problem is that reaching corresponding function is not always obvious - sometimes it's Parser trait, sometimes it's ParseAny, and so on. At RustConf I spoke to Ed Page, clap's maintainer and he gave me an idea to have most of the things as methods on a single structure with different parameters. I implemented that in concrete2 branch and it seem to work. Now all the functions are living in a single namespace.

But current documentation also contains a bunch of cross references - "if you are using this method - see also those methods", etc. And rustdoc can't tell the difference between [Foo<Bar>::help] and [Foo<NotBar>::help] so I can't replicate this cross linking. I tried poking around rustdoc bugtracker - there's a ticket, I tried asking at their zulip - what can I do to get that ticket moving - answer was inconclusive. So now it's either to poke them harder or abandon some of the crosslinking and blame rustdoc. Or to do some of it from my side...