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

No default_value_os for reliably specifying filesystem paths as default arguments #849

Closed
ssokolow opened this issue Feb 12, 2017 · 5 comments · Fixed by #941
Closed

No default_value_os for reliably specifying filesystem paths as default arguments #849

ssokolow opened this issue Feb 12, 2017 · 5 comments · Fixed by #941
Milestone

Comments

@ssokolow
Copy link

ssokolow commented Feb 12, 2017

Affected Version of clap

2.20.3

Expected Behavior Summary

Functions like validator_os and value_of_os should be complemented with a default_value_os.

Actual Behavior Summary

Only default_value and its variants are provided... which means that it's not possible to specify all valid paths as default values.

Sample Code or Link to Sample Code

// env::current_dir is fallible and default_value can't take a callback for lazy eval, so
// resort to "." but future-proof it in case of esoteric platforms.
// (Not perfect, but to_string_lossy() is necessary without a `default_value_os`)
let lazy_currdir = &Component::CurDir.as_os_str().to_string_lossy();

let matches = App::new(env!("CARGO_PKG_NAME"))
    .version(crate_version!())
    // .about("Description text here")
    // TODO: Add args to control slog logging level
    .arg(arg::with_name("inpath")
        .help("file(s) to use as input")
        .multiple(true)
        .empty_values(false)
        .default_value(lazy_currdir)
        // .validator_os(validators::path_readable)
        .required(true))
    .get_matches();

(It's an excerpt from the "new project" boilerplate I'm putting together)

@kbknapp
Copy link
Member

kbknapp commented Feb 12, 2017

While I agree there could be a default_value_os and I'm not opposed to adding it, do you actually need to add a path with invalid UTF-8? I know it's possible, but that doesn't mean it's practical. I'm asking for my own curiosity 😉

@kbknapp kbknapp added this to the 2.21.0 milestone Feb 12, 2017
@ssokolow
Copy link
Author

At the moment, it's more a matter of wanting correctness in how I glue the "get me the current directory" APIs to clap and wanting the APIs I deal with to be complete.

However, there is actually a set of situations where I expect this to be a problem. Some of the projects I have on hold (currently partially prototyped in old Python code using optparse) embed a shell in a way where default values can get very dynamic... and I intend to use them on folders where round-tripping through different distros over the years has not been kind.

(ie. Folders full of files with Japanese filenames names so mojibake'd that they're not even valid UTF-8)

As-is, to play it safe in those projects, I'll be setting a dummy default value (probably \0) and then reinventing defaulting using match if I don't have default_value_os.

kbknapp added a commit that referenced this issue Feb 21, 2017
One can now define default values that contain invalid UTF-8.

The underlying implementation has also been changed to use OsStrs in order to avoid duplication
of code and provide the new APIs basically for free.

Closes #849
@homu homu closed this as completed in 0f2a378 Feb 21, 2017
@ssokolow
Copy link
Author

ssokolow commented Apr 6, 2017

I finally got around to looking at this and it seems you implemented default_value_if_os and default_value_ifs_os but, despite the commit message saying it should be there, you neglected to implement default_value_os.

I can't find any evidence of it in the diff and here are the search results from locally-built docs for clap 2.23.1:
2017-04-06-180023_956x248_scrot

@kbknapp
Copy link
Member

kbknapp commented Apr 6, 2017

Oh wow!

@kbknapp kbknapp reopened this Apr 6, 2017
@kbknapp
Copy link
Member

kbknapp commented Apr 6, 2017

Thanks for letting me know, I'll get this added shortly!

homu added a commit that referenced this issue May 5, 2017
api(Arg): add `default_value_os`

Also add related tests, and reframe `default_value` in terms of
`defualt_value_os`.

This resolves #849.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/kbknapp/clap-rs/941)
<!-- Reviewable:end -->
@homu homu closed this as completed in #941 May 5, 2017
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.

2 participants