Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive attribute to make command enum #178

Open
imsamuka opened this issue Mar 5, 2023 · 6 comments
Open

Derive attribute to make command enum #178

imsamuka opened this issue Mar 5, 2023 · 6 comments

Comments

@imsamuka
Copy link

imsamuka commented Mar 5, 2023

I searched a way to do this but i couldn't. I may have missed it, since the derive docs and naming are very confusing, at least for me.

What i want is a macro attribute (e.g. command_enum) as the other Generated function types that makes the following conversion:

Right now (version 0.7.2)

#[derive(Debug, Clone, Bpaf)]
pub enum MyCommand {
    /// List your accounts
    #[bpaf(command)]
    Accounts,
    /// Login into a account
    #[bpaf(command)]
    Login,
    /// Logout of a account
    #[bpaf(command)]
    Logout {
        no_save: bool
    },
    /// Change status
    #[bpaf(command)]
    Status,
    /// Force save current state
    #[bpaf(command)]
    Save,
}

Proposed

#[derive(Debug, Clone, Bpaf)]
#[bpaf(command_enum)]
pub enum MyCommand {
    /// List your accounts
    Accounts,
    /// Login into a account
    Login,
    /// Logout of a account
    Logout {
        no_save: bool
    },
    /// Change status
    Status,
    /// Force save current state
    Save,
}
@pacak
Copy link
Owner

pacak commented Mar 5, 2023

I may have missed it, since the derive docs and naming are very confusing, at least for me.

Sorry about that, I'm trying to improve the docs as I can, I guess I should go over the derive tutorial as well if people are using it as an entry point to derive API.

Either way - naming used by derive macro should match names used by combinatoric API.

As for the proposal - am I correct that this is a way to avoid typing #[command] every time? Those notations are not quite equivalent - command takes an optional name allowing you to override a command name:

pub enum MyCommand {
    /// List your accounts
    #[bpaf(command("list")))]
    Accounts,

@imsamuka
Copy link
Author

imsamuka commented Mar 6, 2023

Sorry about that, I'm trying to improve the docs as I can, I guess I should go over the derive tutorial as well if people are using it as an entry point to derive API.

No worries, I'm already glad it's even there, that's good work. In a matter of feedback, which gets me the most is trying to put a parser inside other parsers.

For example i tried using struct NoSave(bool) as a better typed, more descriptive argument, but well:

#[derive(Debug, Clone, Bpaf)]
pub enum MyCommand {
    /// Logout of a account
    #[bpaf(command)]
    Logout(
        /* should i put documentation here? */ #[bpaf(external(no_save))] NoSave,
    ),
}

/* or maybe here? */
#[derive(Debug, Clone, Bpaf)]
pub struct NoSave {
    /* or here instead? */ no_save: bool,
}

// i can't manage to compile this
// even if it's just an positional argument
#[derive(Debug, Clone, Bpaf)]
pub struct NoSave(bool);

This is so bad, i actually abuse #[bpaf(external(NAME))] and call a function executing the combinatoric api for everything too complex. Also, in general, the macro also have a lot of trouble with tuple structs, and it isn't very well documented how to execute the workarounds.

Either way - naming used by derive macro should match names used by combinatoric API.

About the naming, i mean things like ParseOptional and OptionParser which are very different things but you can't really tell at a glance.

am I correct that this is a way to avoid typing #[command] every time?

Yes. If you want to do this type of enum, you have to make pretty sure you aren't forgetting a variant without #[command]. I don't know, in fact, if it makes sense to have some variants with #[command] and variants without it; i can't really imagine a use case for this.

Those notations are not quite equivalent - command takes an optional name allowing you to override a command name

Then could we use them optionally? something like this:

#[derive(Debug, Clone, Bpaf)]
#[bpaf(command_enum)]
pub enum MyCommand {
    /// List your accounts
    #[bpaf(command("list")))]
    Accounts,
    /// Login into a account
    Login,
    /// Logout of a account
    Logout {
        no_save: bool
    },
    /// Change status
    Status,
    /// Force save current state
    #[bpaf(command("exit")))]
    Save,
}

We can use other attribute than command when only naming commands, to avoid confusion, like rename or name.

Also, this command_enum shouldn't be mutually exclusive with options and command.

// generates a `my_command() -> impl Parser<MyCommand>`
#[derive(Debug, Clone, Bpaf)]
#[bpaf(command_enum)]
pub enum MyCommand {}

// generates a `my_command() -> ParseCommand<MyCommand>`
#[derive(Debug, Clone, Bpaf)]
#[bpaf(command_enum, command)]
pub enum MyCommand {}

// generates a `my_command() -> OptionParser<MyCommand>`
#[derive(Debug, Clone, Bpaf)]
#[bpaf(command_enum, options)]
pub enum MyCommand {}

I'm sorry if it's too hard to implement, i don't actually know how it's done yet.

@pacak
Copy link
Owner

pacak commented Mar 7, 2023

For example i tried using struct NoSave(bool) as a better typed, more descriptive argument, but well:

documentation goes to /* or here instead? */ - documentation is part of the primitive parser which gets created there, external simply refers to it.

i can't manage to compile this even if it's just an positional argument

This is somewhat intentional, I guess I'll improve improve the error message. Having positional bool mean user will have to type literally true or false - usually programs implement such things with flags. Anyway, if having it positional is exactly what you want this should work

#[derive(Debug, Clone, Bpaf)]
pub struct NoSave(#[bpaf(positional)] bool);

Also, in general, the macro also have a lot of trouble with tuple structs, and it isn't very well documented how to execute the workarounds

would be great if you show me any specific issues you had, at the moment documentation contains this:

With no consumer annotations bpaf_derive parses tuple structs (struct Config(String)) as positional items, but it's possible to override it by giving it a name:

followed by an example.

About the naming, i mean things like ParseOptional and OptionParser which are very different things but you can't really tell at a glance.

Hmm... The idea was to have ParseXXX to be implementation details for impl Parser return values and OptionParser to be the only concrete user facing type but then I had to add catch and a few other modifiers to more parsers and exposed them to hold the documentation... Maybe that was a mistake :)

Yes. If you want to do this type of enum, you have to make pretty sure you aren't forgetting a variant without #[command].

I see, I need to think on that a bit more, it doesn't fit very well

Made this for now: #179

@imsamuka
Copy link
Author

imsamuka commented Mar 7, 2023

It seems like bool is a edge case, and i didn't notice while writing the example, because i tried (#[bpaf(positional)] bool), and it doesn't compile (derive version 0.3.4). Other types work.

But sorry, i was mistaken about the NoSave(bool), i didn't actually wanted a positional argument there, i should have used #[bpaf(long("no-save")]. Bad example on my part.

Overall, I think it boils down to just have more common practical examples in the documentation. In the section 5 of the docs, it explains "consumer section corresponds to argument, positional, flag, switch and any." but there's only a example using argument, and in another section. Reading about how everything work is different than seeing it work. Or maybe that's just something fundamentally wrong about how I read lib documentation, I tend to focus more on examples and snippets, and then expand from it. If they aren't inlined in the documentation, direct links to the repository examples would be nice too, showing specific examples to the section.

would be great if you show me any specific issues you had ...

  • Named positional argument example:
#[derive(Debug, Clone, Bpaf)]
struct Path(#[bpaf(positional("PATH"))] PathBuf);
  • Tuple struct documentation example:
#[derive(Debug, Clone, Bpaf)]
struct Path(#[doc = "path used in the program"] PathBuf);
  • Examples for argument, flag, any, parse, map, req_flag; All cited in the docs, but without obvious examples, especially map.

and OptionParser to be the only concrete user facing type

I'm curious, why did you named it like this? I mean, it could be like BuiltParser or FinalParser. It's a historical decision maintained because of the API?

@pacak
Copy link
Owner

pacak commented Mar 7, 2023

and it doesn't compile (derive version 0.3.4)

It's fixed in master, I guess I need to publish it :)

Overall, I think it boils down to just have more common practical examples in the documentation.

I see. Documentation might not be directly in the tutorial section, but pretty much every function comes with examples with both APIs and how it interacts with real command line inputs - they are generated and tested, see this for example, try clicking at lines marked with triangle:

https://docs.rs/bpaf/latest/bpaf/trait.Parser.html#method.map

It also lists 12 different fully implemented parsers from examples folder.

It's a historical decision maintained because of the API?

Mostly historical

@imsamuka
Copy link
Author

imsamuka commented Mar 7, 2023

I see. Documentation might not be directly in the tutorial section, but pretty much every function comes with examples with both APIs and how it interacts with real command line inputs - they are generated and tested, see this for example, try clicking at lines marked with triangle:
https://docs.rs/bpaf/latest/bpaf/trait.Parser.html#method.map

Wow, i didn't notice how rich the docs.rs normal documentation was; the tutorial don't come even close!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants