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

Implement helper methods for PathBufValueParser #4686

Closed
wants to merge 1 commit into from

Conversation

bim9262
Copy link

@bim9262 bim9262 commented Jan 30, 2023

Resolves a few of the use cases outlined in #4074, albeit with different function names. The function names match the function names of PathBuf.

Resolves a few of the use cases outlined in clap-rs#4074.
Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting to you on this.

As I mentioned in the Issue, I was thinking this would likely be best to start off as a clap_fs crate, rather than directly supporting this in clap.

If you go this route, it would help if you split the commits to make this easy to review

  • One commit just adding an empty clap_fs crate, following the pattern of other crates
  • One commit adding the desired functionality with tests and examples

If you would like to further discuss design ideas (as compared to the implementation), let's do that over on the issue.

Comment on lines +999 to +1000
option_set_func!(String, (starts_with, ends_with));
option_set_func!(
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer we not use macros for all of the functions as it obscures what is happening

Comment on lines +913 to +914
starts_with: Option<String>,
ends_with: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing these defined in click. Let's hold off on these in this PR and instead discuss their use case in the issue

@bim9262
Copy link
Author

bim9262 commented Feb 24, 2023

Sounds good @epage! I'll take a look into that 😄

@epage
Copy link
Member

epage commented Jul 21, 2023

Closing in favor of clio for now. If there is further work you'd like to see done to clio, feel free to open issues there. If you think we should go in a different direction, then you are welcome to comment on #4074.

@epage epage closed this Jul 21, 2023
@bim9262
Copy link
Author

bim9262 commented Jul 23, 2023

Thanks for the heads up! I was in the process of trying to create a creat to do this as you'd suggested, but it looks like this will work 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