mktemp: add func to expose functionality (for use in nushell) by tskinn · Pull Request #5479 · uutils/coreutils

tskinn · 2023-10-30T15:12:00Z

Tried to follow @tertsdiepraam guidance here

Link to #5465

tertsdiepraam

Exactly how I imagined it! Thanks! I just have a few additional suggestions.

tertsdiepraam · 2023-10-30T15:20:28Z

src/uu/mktemp/src/mktemp.rs

+}
+
+pub fn mktemp(
+    dir: &str,


Would it be possible to make this into a &Path?

Yeah sounds good

tertsdiepraam · 2023-10-30T15:21:45Z

src/uu/mktemp/src/mktemp.rs

 }

-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> {


Do you happen to know why this is pub?

Nope. It did seem a little odd.

tertsdiepraam · 2023-10-30T15:22:27Z

src/uu/mktemp/src/mktemp.rs

+    Ok(path)
+}
+
+pub fn mktemp(


Could you give this a bit of documentation?

tskinn · 2023-10-30T23:22:18Z

So sorry! I should have had a quick try at implementing mktemp in nushell before bothering you.

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 mkdir function. None of the actual logic should have changed.

Basically what changed

removed pub from dry exec
created a new func for Options struct containing the logic for getting the system tmp dir which is reused in the original from function
exec functions don't print but return result with path
uumain prints to stdout
added new mktemp function

Now that I'm looking back I can probably cleanup the arguments to the mktemp function. Going to do that.

tskinn · 2023-10-31T15:15:18Z

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.

tertsdiepraam

Nice work! I think it should be done slightly differently, but it's very much going in the right direction, see my other comment. I hope my description is clear, if it's not, feel free to ask for clarification!

tertsdiepraam · 2023-11-01T07:52:38Z

src/uu/mktemp/src/mktemp.rs

            }
            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 {


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:

coreutils/src/uu/cp/src/cp.rs

Line 224 in 615b562

pub struct Options {

The mktemp function would then also take the Options struct instead of all the separate parameters.

tertsdiepraam

Great! Just one last small suggestion

tertsdiepraam · 2023-11-02T13:12:42Z

src/uu/mktemp/src/mktemp.rs

    // 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)


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?

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.

github-actions · 2023-11-07T06:26:52Z

GNU testsuite comparison:

Congrats! The gnu test tests/tail/retry is no longer failing!
Congrats! The gnu test tests/tail/symlink is no longer failing!

tertsdiepraam · 2023-11-07T10:41:07Z

src/uu/mktemp/src/mktemp.rs


        Ok(Self {
-            directory,
+            directory: directory.into(),


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.

tertsdiepraam · 2023-11-07T10:43:40Z

Thanks! Hope to see you on the nushell side to implement that part!

…#5479) * mktemp: add func to expose functionality * mktemp: cleanup

cakebaker linked an issue Oct 30, 2023 that may be closed by this pull request

mktemp: expose functionality for nushell #5465

Closed

tertsdiepraam reviewed Oct 30, 2023

View reviewed changes

tskinn force-pushed the main branch from 6d110c2 to b7009e8 Compare October 30, 2023 23:15

tskinn force-pushed the main branch from b7009e8 to 4cb608b Compare October 31, 2023 15:00

tertsdiepraam reviewed Nov 1, 2023

View reviewed changes

tskinn force-pushed the main branch from 4cb608b to 4669d52 Compare November 1, 2023 22:29

mktemp: add func to expose functionality

83cc22f

tskinn force-pushed the main branch from 4669d52 to 83cc22f Compare November 1, 2023 23:22

tertsdiepraam reviewed Nov 2, 2023

View reviewed changes

mktemp: cleanup

03dac11

tertsdiepraam approved these changes Nov 7, 2023

View reviewed changes

tertsdiepraam merged commit 6678c17 into uutils:main Nov 7, 2023

tuxdna pushed a commit to tuxdna/coreutils that referenced this pull request Nov 8, 2023

mktemp: add func to expose functionality (for use in nushell) (uutils…

6b15061

…#5479) * mktemp: add func to expose functionality * mktemp: cleanup

tskinn mentioned this pull request Nov 10, 2023

Add mktemp command nushell/nushell#11005

Merged

chenrui333 mentioned this pull request Nov 14, 2023

uutils-coreutils 0.0.23 Homebrew/homebrew-core#154288

Merged

moonfruit mentioned this pull request Nov 17, 2023

uutils-selected 0.0.23 moonfruit/homebrew-tap#59

Merged

Uh oh!

Conversation

tskinn commented Oct 30, 2023

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tskinn commented Oct 30, 2023

Uh oh!

tskinn commented Oct 31, 2023

Uh oh!

tertsdiepraam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tertsdiepraam left a comment •

edited

Loading