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

file() verification should be able to expand a leading tilde (~) #131

Closed
sschuberth opened this issue Feb 17, 2020 · 11 comments · Fixed by #132
Closed

file() verification should be able to expand a leading tilde (~) #131

sschuberth opened this issue Feb 17, 2020 · 11 comments · Fixed by #132

Comments

@sschuberth
Copy link
Contributor

file() currently fails for a path like ~/my-file.txt as ~ is not expanded. It would be great if ~ would be expanded when running on shells that support it similar to like we do here, maybe by introducing an expandTilde parameter to file() which defaults to true.

@sschuberth
Copy link
Contributor Author

sschuberth commented Feb 17, 2020

Or maybe this should be configurable at the Context-level similar to expandArgumentFiles?

Edit: But I have a slight preference to configure this on the file()-level.

@sschuberth
Copy link
Contributor Author

BTW, I tried to work around the issue by doing

private val inputDir by option(
    "--input-dir", "-i",
    help = "The project directory to analyze."
).convert {
    File(it).expandTilde().path
}.file(exists = true, fileOkay = false, folderOkay = true, writable = false, readable = true).required()

but that did not work as convert() does not seem to be called before file()...

@ajalt
Copy link
Owner

ajalt commented Feb 17, 2020

So the problem is that checks like exists = true fail because they can't resolve paths that start with ~?

It seems better to pass through what the user entered unchanged, but we should definitely expand tildes while we do the checks.

@sschuberth
Copy link
Contributor Author

So the problem is that checks like exists = true fail because they can't resolve paths that start with ~?

Yes. I get Error: Invalid value for "-i": Directory "~/Downloads/test" does not exist.

@ajalt
Copy link
Owner

ajalt commented Feb 17, 2020

What operating system and shell do you see this problem on? I expect that it's an issue in all cases, but it would be good to know if it's only affecting e.g. windows, which doesn't use the ~ convention.

@sschuberth
Copy link
Contributor Author

sschuberth commented Feb 18, 2020

Yeah, I should have explained the use-case in more detail, apologies.

Obviously everything is fine if an app using clikt is called from a regular command line prompt with a shell that supports tilde-expansion, as the shell performs the expansion before the app sees the path. The issue occurs when calling such an app programmatically, like from an IntelliJ IDEA Run Configuration. Out of habit I'm sometimes using paths with a ~ as arguments in such a Run Configuration, but as no shell is involved in that case, no tilde-expansion happens. In these cases the tilde-expansion needs to be performed by the app.

I'd be perfectly fine if you say this is a non-feature that you would not like to add, but in that case I'd appreciate a hint why my work-around that chains convert() and file() does not work.

@ajalt
Copy link
Owner

ajalt commented Feb 18, 2020

Thanks for the clarification; the issue makes sense now.

We can definitely expand tildes automatically when doing exists checks etc. I think an expandTildes parameter is a bit too niche, so I'd rather just let callers do the expansion if they need to.

To answer your last question: file is an extension on Option<String?>, and you're trying to call it on an Option<File?>, which is what you're returning from convert. I recently added wrapValue for use cases like yours, which can could use like option().file().wrapValue {it.expandTilde()}

@ajalt ajalt linked a pull request Feb 18, 2020 that will close this issue
@sschuberth
Copy link
Contributor Author

and you're trying to call it on an Option<File?>

Are you sure? File(it).expandTilde().path actually returns a String, so convert { File(it).expandTilde().path } should return Option<String?>, no?

@ajalt
Copy link
Owner

ajalt commented Feb 18, 2020

Sorry about that, I misread your code. The reason it doesn't work is because convert (which file and all conversions call) replaces the existing conversion. So in your case the block passed to convert wouldn't get called, since file overwrites it.

The type system normally prevents you from calling convert more than once, since it convert usually changes the type of the option. But if convert returns a String, the types stays the same and this mistake is possible to make. Maybe a future version of Clikt will be more strict with they types, but that's a serious breaking change.

@sschuberth
Copy link
Contributor Author

I recently added wrapValue for use cases like yours, which can could use like option().file().wrapValue {it.expandTilde()}

Right, I remember about that, however that does not seem to help here as the wrapping (and thus the tilde expansion) seems to happen after the conversion to File and the existence check, so I cannot really use that as a work-around, unless I'm mistaken.

@ajalt
Copy link
Owner

ajalt commented Feb 19, 2020

That's correct. I implemented the tilde expansion for file checks in #132, so you can use it with snapshot 2.5.0-beta1.387-SNAPSHOT or newer if you need the functionality now. Otherwise I'm planning a release within the next week.

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