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

swap structopt for bpaf #57

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

swap structopt for bpaf #57

wants to merge 4 commits into from

Conversation

pacak
Copy link

@pacak pacak commented Nov 29, 2022

Clean build is a bit faster and a bit smaller

before:
cargo build --release  70.39s user 3.50s system 823% cpu 8.976 total
% ls -lha target/release/choose                                                                  
-rwxrwxr-x 2 pacak pacak 6.1M Nov 28 20:42 target/release/choose
                                                                    
after:
cargo build --release  45.35s user 1.96s system 611% cpu 7.731 total
% ls -lha target/release/choose                                                                  
-rwxrwxr-x 2 pacak pacak 5.7M Nov 28 20:43 target/release/choose

I also removed vector allocation from tests - should be faster since bpaf can take them as a reference and dropped the program name from all the arguments - it is handled automagically.

Also added FromStr impl for Choice and replaced ToString for ParseError with std::fmt::Display.

You might want to expose bpaf's bright-color / dull-color for cosmetic reasons. And maybe autocomplete.

It should be possible to speed up the compilation a bit more if we replace derive API with combinatoric one.

@lukehsiao
Copy link

I notice the help generates a strange warning.

❯ cargo run -- --help
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/choose --help`
failed to parse choice argument: --help
`choose` sections from each line of files

Usage: [-c] [-d] [-x] [-f ARG] [-i INPUT] [-n] [--one-indexed] [-o SEP] <CHOICE>...

Available positional items:
    <CHOICE>  Fields to print. Either a, a:b, a..b, or a..=b, where a and b are integers. The beginning
              or end of a range can be omitted, resulting in including the beginning or end of the line,
              respectively. a:b is inclusive of b (unless overridden by -x). a..b is
              exclusive of b and a..=b is inclusive of b.

Available options:
    -c, --character-wise                Choose fields by character number
    -d, --debug                         Activate debug mode
    -x, --exclusive                     Use exclusive ranges, similar to array indexing in many programming languages
    -f, --field-separator <ARG>         Specify field separator other than whitespace, using Rust `regex` syntax
    -i, --input <INPUT>                 Input file
    -n, --non-greedy                    Use non-greedy field separators
        --one-indexed                   Index from 1 instead of 0
    -o, --output-field-separator <SEP>  Specify output field separator
    -h, --help                          Prints help information

Specifically, the

failed to parse choice argument: --help

But besides that, this seems awesome!

This PR is very educational just for me to see the derive API implemented in this way from the author themselves. I took a swing with bpaf earlier, but couldn't get it quite working out.

@pacak
Copy link
Author

pacak commented Nov 29, 2022

failed to parse choice argument: --help

Hmm.... choice function shouldn't really print anything to stderr if it also wants to return an error, bpaf will print whatever error it returns.

src/opt.rs Outdated Show resolved Hide resolved
@pacak
Copy link
Author

pacak commented Nov 29, 2022

I took a swing with bpaf earlier, but couldn't get it quite working out.

I would really like to see some of that along with the comments of what you tried to do, how and why so I can improve the documentation or maybe the library. Here or there's a ticket in bpaf repo with "feedback wanted" label.

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

Successfully merging this pull request may close these issues.

2 participants