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

Thread Crash/Panic #110

Closed
sebasptsch opened this issue Feb 22, 2023 · 6 comments
Closed

Thread Crash/Panic #110

sebasptsch opened this issue Feb 22, 2023 · 6 comments

Comments

@sebasptsch
Copy link

pipes-rs --palette pastel --kinds curved -i true -p 5

thread 'main' panicked at 'Mismatch between definition and access of `palette`. Could not downcast to TypeId { t: 935915684833699536 }, need to downcast to TypeId { t: 14429720537726057407 }
', /home/sebas/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/clap-4.1.6/src/parser/error.rs:30:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@lunacookies
Copy link
Collaborator

Clap v4 must’ve changed how #[arg(value_parser)] works, since not only does --palette with any correct value trigger this panic, but so do --color-mode, --bold and --inherit-style. Clap has now released v5. Rather than erroneously upgrading without proper testing as happened with Clap v4, I propose we roll our own CLI argument parsing. @lhvy How do you feel about this?

@sebasptsch
Copy link
Author

@lhvy ?

@Nukesor
Copy link

Nukesor commented Feb 26, 2023

Clap v4 must’ve changed how #[arg(value_parser)] works, since not only does --palette with any correct value trigger this panic, but so do --color-mode, --bold and --inherit-style. Clap has now released v5. Rather than erroneously upgrading without proper testing as happened with Clap v4, I propose we roll our own CLI argument parsing. @lhvy How do you feel about this?

After reading your comment, I was wondering why they already created a v5.
But I couldn't find a v5 and there also doesn't seem to be a yanked release (the last one was v3.0.0-beta.3)
https://crates.io/crates/clap/versions

I'm not involved into this project and just stumbled accross this issue, but rolling your own CLI argument parsing sounds like a bad idea. Clap recently did many refactorings and major breaking changes in order to get their API cleaned up and I would say they managed to do so. The crate is now much more mature, they managed to hide unwanted complexity behind feature flags and streamlined their API.

Writing your own implementation of something hundreds of people racked their brains about will just resolve in much more maintenance work and new bugs for this project. IMO the better cause of action would be to help out with the development of clap instead.

@lunacookies
Copy link
Collaborator

lunacookies commented Feb 27, 2023

After reading your comment, I was wondering why they already created a v5. But I couldn't find a v5 and there also doesn't seem to be a yanked release (the last one was v3.0.0-beta.3) https://crates.io/crates/clap/versions

You are, of course, completely right :) Turns out I saw v5 in the changelog, but yeah it looks like they haven’t released v5 anywhere. I’m not completely sure what’s up with that, though I’m sure there’s an explanation.

Clap recently did many refactorings and major breaking changes in order to get their API cleaned up and I would say they managed to do so. The crate is now much more mature, they managed to hide unwanted complexity behind feature flags and streamlined their API.

Thanks for the info! That’s good news, I’ll have to take a look at the changes.

Writing your own implementation of something hundreds of people racked their brains about will just resolve in much more maintenance work and new bugs for this project. IMO the better cause of action would be to help out with the development of clap instead.

Respectfully, I disagree. The tiny subset of Clap’s functionality strictly necessary for pipes-rs (short options, long options) is trivial to implement, and the rest isn’t much effort (Levenshtein distance-based typo correction, colored help output).

@lunacookies
Copy link
Collaborator

@lhvy It occurred to me that, in the over-engineered spirit of this project, it might be nice to add automated tests for the command-line interface so these sorts of bugs can be caught in the future.

@lhvy
Copy link
Owner

lhvy commented Mar 17, 2023

Sure. I should have some time soon. Should we decide on what we want to do from here and then fix this up?

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 a pull request may close this issue.

4 participants