Skip to content

Migrate to bpaf command line parsing and completions#239

Draft
Coca162 wants to merge 1 commit into
andir:masterfrom
Coca162:bpaf
Draft

Migrate to bpaf command line parsing and completions#239
Coca162 wants to merge 1 commit into
andir:masterfrom
Coca162:bpaf

Conversation

@Coca162
Copy link
Copy Markdown
Collaborator

@Coca162 Coca162 commented May 31, 2026

Lots of cleanup needed, I'm also not happy with how there is some bugginess and differences in the completions compared to with the native fish completions. There is also no help subcommands currently and the help formatting is different in ways I minorly dislike. All flags are global as well, though this is not indicated in help. Hopefully some of these can be dealt with upstream.

Otherwise this was library was pretty fun to work with even including the relatively cursed way to share the lockfile to completions. Combinatorics are much nicer then what clap offers imo.

@Coca162 Coca162 force-pushed the bpaf branch 3 times, most recently from 5a9ea20 to 2659268 Compare May 31, 2026 14:43
@Coca162
Copy link
Copy Markdown
Collaborator Author

Coca162 commented May 31, 2026

Changing the nix tests so that npins --lock-file sources.json -d npins isn't accepted anymore, since bpaf can easily handle directory having a default and being mutually exclusive with another flag unlike clap

@pacak
Copy link
Copy Markdown

pacak commented Jun 2, 2026

o/

Upstream here :)

I'm also not happy with how there is some bugginess and differences in the completions compared to with the native fish completions.

Not happy with that either. Originally I had it generating native fish completions, turns out fish caches them between runs instead of asking for a new set every time leading to wrong completions being offered. There was a ticket open about this specific issue and it was closed with "well, nobody does dynamic completions like that so current behavior is fine". Duh. I wonder why nobody does it....

But that was in C++ days, maybe it behaves differently now. Will revisit at some point now that I have more robust testing system for completions in place.

There is also no help subcommands currently

You can make one yourself.

To support stuff like npins help add tarball you can add a variant with command help and consume everything in the body with any or something, then do OptionParser::run_inner on the input with --help added at the back. Or instead of adding help to your commands make a wrapper like enum HelpOrCmd { Help(Vec<OsString>), Command(Command)`.

and the help formatting is different in ways I minorly dislike.

Kind of hard to do something about it at the moment. Slowly reworking some bits to make it easier to tweak it, but no ETA promises.

All flags are global as well, though this is not indicated in help.

Same as with changing help formatting. There's something that works, but I'm planning to revisit most of the documentation. No ETA.

Hopefully some of these can be dealt with upstream.

Some :)

Otherwise this was library was pretty fun to work

❤️

with even including the relatively cursed way to share the lockfile to completions.

Should become less cursed in the next major release (still no ETA)

Combinatorics are much nicer then what clap offers imo.

Lax Monoidal Functors FWIW.

Comment thread src/opts.rs Outdated
Comment on lines +34 to +36
let normal = pure(Self::Normal);

construct!([full, partial, normal])
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should give the same parsing behavior, but slightly better error messages if something goes wrong.

Suggested change
let normal = pure(Self::Normal);
construct!([full, partial, normal])
construct!([full, partial]).fallback(Self::Normal)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Comment thread src/opts.rs Outdated
pub repository: String,

#[command(flatten)]
// #[command(flatten)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to use derive macro - it would be #[bpaf(external)]

Comment thread src/opts.rs Outdated
})
})
.fallback(GitForgeOpts::Auto)
.format_fallback(|_, f| f.write_str("auto"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.display_fallback() will probably do the same if for GitForgeOpts::Auto std::fmt::Display says "auto".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep yep, just went for the quickest route here but might as well

Comment thread src/opts.rs Outdated
let github = construct!(GitHubAddOpts {
more(GenericGitAddOpts::parser()),
owner(positional("OWNER")),
repository(positional("REPOSITORY"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax is going away in the next big release (no ETA), I think it's confusing after all.

Comment thread src/opts.rs Outdated
at(long("at").argument("VERSION").help("Use a specific release instead of the latest.").optional()),
version_upper_bound(long("upper-bound").argument("VERSION").help(r#"Bound the version resolution. For example, setting this to "2" will restrict updates to 1.X versions."#).optional()),
package_name(positional("PACKAGE").help("Name of the package at PyPi.org"))
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that's the example why I don't like this syntax :)

Compact, but no rust-analyzer, no rust-fmt, etc. Having a few variables defined outside and referring them from the macro is cleaner

let at = long("at")... 
...
let pypi = construct!(PyPiAddOpts { at, version_upper_bound, package_name });`

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temptation of trying to make it concise got to me! I definitely agree with your assessment that its a bit scuffed though, I have similar reasons as to why I don't like derive macros as well.

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