mktemp: add func to expose functionality (for use in nushell) #5479
Conversation
src/uu/mktemp/src/mktemp.rs
Outdated
| } | ||
|
|
||
| pub fn mktemp( | ||
| dir: &str, |
There was a problem hiding this comment.
Would it be possible to make this into a &Path?
src/uu/mktemp/src/mktemp.rs
Outdated
| @@ -441,7 +442,7 @@ pub fn uu_app() -> Command { | |||
| .arg(Arg::new(ARG_TEMPLATE).num_args(..=1)) | |||
| } | |||
|
|
|||
| pub fn dry_exec(tmpdir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult<()> { | |||
| pub fn dry_exec(tmpdir: &str, prefix: &str, rand: usize, suffix: &str) -> UResult<PathBuf> { | |||
There was a problem hiding this comment.
Do you happen to know why this is pub?
There was a problem hiding this comment.
Nope. It did seem a little odd.
src/uu/mktemp/src/mktemp.rs
Outdated
| Ok(path) | ||
| } | ||
|
|
||
| pub fn mktemp( |
There was a problem hiding this comment.
Could you give this a bit of documentation?
|
So sorry! I should have had a quick try at implementing After doing so I realized I wasn't exposing the stuff I needed. Since there is a little bit of logic around getting the system tmp dir I just rearranged where that was done exactly so we could reuse that in the pub Basically what changed
Now that I'm looking back I can probably cleanup the arguments to the |
|
Ok @tertsdiepraam let me know what you think. I'm a rust noob so I'm sure theres lots to change. I can get nushell mktemp working with this api at least. |
src/uu/mktemp/src/mktemp.rs
Outdated
| // If no template argument is given, `--tmpdir` is implied. | ||
| None => { | ||
| let tmpdir = Some(tmpdir.unwrap_or_else(env::temp_dir)); | ||
| let template = DEFAULT_TEMPLATE; | ||
| (tmpdir, template.to_string()) | ||
| } | ||
| Some(template) => { | ||
| let tmpdir = if env::var(TMPDIR_ENV_VAR).is_ok() && matches.get_flag(OPT_T) { | ||
| let tmpdir = if env::var(TMPDIR_ENV_VAR).is_ok() && treat_as_template { |
There was a problem hiding this comment.
Nushell has their own env var handling, so I think that what we expose should not depend on env vars. Basically what I think should happen is that this method should not have been changed, but the Options struct should be exposed, so that they can construct it however they want. Just like how we did it in cp:
Line 224 in 615b562
The mktemp function would then also take the Options struct instead of all the separate parameters.
src/uu/mktemp/src/mktemp.rs
Outdated
| @@ -352,17 +353,18 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { | |||
|
|
|||
| // Create the temporary file or directory, or simulate creating it. | |||
| let res = if dry_run { | |||
| dry_exec(&tmpdir, &prefix, rand, &suffix) | |||
| dry_exec(Path::new(&tmpdir), &prefix, rand, &suffix) | |||
There was a problem hiding this comment.
You don't have to fix this here, but could you add a TODO saying that we should get a PathBuf from clap instead of String?
There was a problem hiding this comment.
Yeah absolutely. Thanks for your patience by the way.
It seems like we are already getting the pathbuf from clap https://github.com/uutils/coreutils/blob/main/src/uu/mktemp/src/mktemp.rs#L413-L430 Maybe I'm looking at the wrong thing here.
But I see we can at least use PathBuf in the Params struct. I'll add a commit for that.
|
GNU testsuite comparison: |
| @@ -297,7 +298,7 @@ impl Params { | |||
| let num_rand_chars = j - i; | |||
|
|
|||
| Ok(Self { | |||
| directory, | |||
| directory: directory.into(), | |||
There was a problem hiding this comment.
Cool that we indeed already get the PathBuf from clap! However, we still need to fix this conversion from String inbetween, but I'll open an issue for that.
|
Thanks! Hope to see you on the nushell side to implement that part! |
…#5479) * mktemp: add func to expose functionality * mktemp: cleanup
Tried to follow @tertsdiepraam guidance here
Link to #5465