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

Add validator_all, validator_all_os #3029

Closed
wants to merge 1 commit into from

Conversation

ctrlcctrlv
Copy link

For MFEKstroke's new dash mode, I needed to be able to write a validator that would check and make sure that the number of values I had for an argument was divisible by two. Unable to do this with e.g. min_values etc. I decided to add new functions validator_all and an _os counterpart as that is "the clap way". Although I could've solved it just for me with validator_num, I believe this is better because it puts the whole issue to bed.

Also added tests for the new functions.

@epage
Copy link
Member

epage commented Nov 15, 2021

I'm a bit unsure what I think. Some random thoughts

While some of those comments are about plans that are probably months out, there is the workaround of handwriting this validator, rather than having clap do it. We've recently exposed App::error or Error::raw with Error::format to get clap-formatted errors for clap users.

To be clear, this is not a "no" but me sharing my thoughts as part of a conversation with you, other clap users, and other clap maintainers.

ps Personally, I recommend issues or discussions for proposing new APIs since PRs couple any conversation we have to their PR when PRs might get created and discarded, depending on how the conversation goes. I wonder if we should encourage that in the CONTRIBUTING.md...

@ctrlcctrlv
Copy link
Author

@epage Thanks for your thoughts; I did not think that this would be without controversy, especially given #2832 where you want to go an entirely different direction than validators. I renamed MFEK/clap to MFEK/clap.rlib (i.e. made it match my org's house style) because I had a feeling I'd be maintaining a clap fork given #2906 (comment). So feel free to say "no", even "hell no", you're not hurting my feelings any.

If you choose to decline this patch as better solved in the future by your changes, either the planned ones in #2832, or some other plan, that's fine. I'll use it for the meanwhile.

@epage
Copy link
Member

epage commented Nov 15, 2021

What are your thoughts on the App::error approach?

It'd look something like

let mut app = ...;
let matches = app.get_matches_mut();

let arg_len = matches.values_of("arg").unwrap().len();
if arg_len % 2 != 0 {
    app.error("--arg's values should be in pairs of two").exit();
}

@ctrlcctrlv
Copy link
Author

I like the fact that I know all validation is done when I have ArgMatches. I expect clap to do all validation of args, so I wouldn't write it that way, but wouldn't think it "wrong" to do so if someone is in a hurry and doesn't feel like using (or in my case making) a patch to clap itself.

@kbknapp
Copy link
Member

kbknapp commented Nov 16, 2021

Personal opinion here; I would move this to post v3 if at all. We should definitely be in a feature freeze until v3 is released. Once it's released, I do expect there to be some discussion on where the line is between what is acceptable to punt to application code, vs what should be handled at the CLI level.

The original intent of validator is to validate a single argument value. Not validating all argument values in their combined forms, as that is a much harder task generically speaking. I do understand how that line isn't clear though, and it's possible clap could use some kind of indicator to assist with paired arguments or total values, etc.

@ctrlcctrlv
Copy link
Author

Do we at least all agree that it's a good general principle of software design to separate concerns, and that the concern of CLI argument parsing belongs to clap?

How we make clap responsible for all parsing a user might need is another question; this is one way, currently working for me (and compiled fork will be distributed in the next version of MFEKstroke, perhaps MFEKpathops and MFEKmetadata, doubt MFEKglif will need it), so there's no rush as far as I'm concerned for us to figure it out. clap is a toplevel dependency…it matters little if I point it at my git repo for a while while we do.

@kbknapp
Copy link
Member

kbknapp commented Nov 17, 2021

Do we at least all agree that it's a good general principle of software design to separate concerns, and that the concern of CLI argument parsing belongs to clap?

Absolutely! I'm not saying we won't include this or something very similar, just that our resources are limited so we have to draw seemingly arbitrary lines between what clap handles and what gets pushed into application code. In this particular case there is a good workaround with minimal user code that can be used while we prioritize the v3 release and then re-evaluate. This particular issue also only tangentially touches a muddy part of the API that I'd love to take a more concerted look at (multiples values and everything they encompass) but that won't happen until v3 is released and that weight is lifted 😉

@epage
Copy link
Member

epage commented Dec 7, 2021

Since it seems we won't be moving forward with this design, I'm going ahead and closing this PR out.

@epage epage closed this Dec 7, 2021
@ctrlcctrlv
Copy link
Author

Hello friends,

Congrats on the releases of 3.0 & 3.0.1!

In commits f48280c and the less important ec5a075, MFEK's clap fork w/validator_all is now tracking you.

I'd like to know if we can discuss this again now that 3.0 is out. I don't want to maintain this fork forever, though of course will if necessary, it's not a huge burden, am just hoping we can find consensus :)

@epage
Copy link
Member

epage commented Jan 4, 2022

Without further input, this is feeling a bit specialized. I would lean towards recommending App::exit until we have #3008. While I agree about a separation of concerns, I do not think that means everything should happen within the App::get_matches call.

Maybe create an issue for this and we can see how much interest this garners.

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.

3 participants