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.

Sign up for GitHub

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

Jump to bottom

Add umkdir command #10785

Merged
merged 2 commits into from Oct 30, 2023
Merged

Add umkdir command #10785

merged 2 commits into from Oct 30, 2023

Conversation

KAAtheWiseGit
Copy link
Contributor

A mkdir command, which uses uu_mkdir as backend.

close #10515.

@KAAtheWiseGit KAAtheWiseGit mentioned this pull request Oct 20, 2023
Closed
@amtoine
Copy link
Member

amtoine commented Oct 20, 2023

@KAAtheWiseGit
in #10515, you say this is a DRAFT 😉

@amtoine amtoine marked this pull request as draft October 20, 2023 14:08
@KAAtheWiseGit
Copy link
Contributor Author

@amtoine, yeah.

I couldn't compile nu-commands because of some openssl issues. So, this PR is hanging in the air now.

I'll probably try adding tests and running the workflows.

@amtoine
Copy link
Member

amtoine commented Oct 20, 2023

I'll probably try adding tests and running the workflows.

let us know when you have tests to run 😊

@KAAtheWiseGit
Copy link
Contributor Author

KAAtheWiseGit commented Oct 20, 2023 •
edited

I just realized I can simply copy the mkdir tests.

The only thing which is not supported by uu_mkdir is umkdir .../boo. I deleted this test case for now, but this is a regression. I don't think it can be merged as-is.

@fdncred fdncred added the coreutils-uutils Changes relating to coreutils/uutils label Oct 20, 2023
@fdncred
Copy link
Collaborator

fdncred commented Oct 20, 2023

What about the mkdirs -p functionality. I think nushell might do that implicitly, does this version?

@amtoine amtoine added the more information needed we need more information from you label Oct 21, 2023
@KAAtheWiseGit
Copy link
Contributor Author

@fdncred, yes, I've enabled it by default. It is controlled by the IS_RECURSIVE constant. And the creates_intermediary_directories test also checks for it.

@KAAtheWiseGit KAAtheWiseGit marked this pull request as ready for review October 21, 2023 12:34
@fdncred
Copy link
Collaborator

fdncred commented Oct 21, 2023

ok, cool. I don't see anything really missing then. Are we ready to land it?

@fdncred fdncred removed the more information needed we need more information from you label Oct 21, 2023
@KAAtheWiseGit
Copy link
Contributor Author

If the mkdir .../a regression is not an issue, yes. If not, I'll need to tweak the code.

@fdncred
Copy link
Collaborator

fdncred commented Oct 22, 2023

ok, thanks. Is there no nushell functionality that will expand ...?

@KAAtheWiseGit
Copy link
Contributor Author

I don't see it. Is this something built-in into the arguments? Also, I noted two more issues in my code, I'll need to fix those.

@sholderbach sholderbach added the file-system Related to commands and core nushell behavior around the file system label Oct 23, 2023
@fdncred fdncred marked this pull request as draft October 23, 2023 11:43
@fdncred
Copy link
Collaborator

fdncred commented Oct 23, 2023 •
edited

Let's make this a draft until all issues are resolved.

Also, I noted two more issues in my code, I'll need to fix those.

Do you mind saying what those are? All I see is the ... expansion. It's early here, maybe I'm just missing them.

BTW - I had no issues compiling this PR.

@fdncred
Copy link
Collaborator

fdncred commented Oct 23, 2023 •
edited

Oh, I see what's going on.

  1. You need to add umkdir to nu-command/src/default_context.rs
  2. You need to add umkdir to nu-command/src/filesystem/mod.rs
  3. You need to make uu_mkdir version 0.0.22 in the cargo.toml
  4. Then there's a few bugs to fix and after that umkdir .../test seems to work fine

@KAAtheWiseGit
Copy link
Contributor Author

Got it!

I also forgot about examples and, quite embarrassingly, proper error wrapping.

@KAAtheWiseGit
Copy link
Contributor Author

Somehow rust-analyzer was able to process the files, despite me still not being able to build Nushell locally (I'll probably open a separate issue about it).

I added examples, uutils tag, three-dot test, and put the command into default_context and filesystem/mod.

@KAAtheWiseGit
Copy link
Contributor Author

So, while I was writing the issue, I realized I didn't have the necessary dependency.

I managed to get the tests running locally, will fix the issues in a moment.

@KAAtheWiseGit
Add umkdir command
469a515 
A `mkdir` command, which uses the `uutils` backend.
@KAAtheWiseGit
Copy link
Contributor Author

Attempt two.

This time print_created_paths doesn't pass. For some reason, umkdir doesn't print anything in verbose mode. I don't understand why.

@KAAtheWiseGit
Copy link
Contributor Author

Both uu_mkdir and uu_cp use println!. Both umkdir and ucp call uutils functions with if let else. There must be something I am doing wrong, but I can't find the cause.

@KAAtheWiseGit
Copy link
Contributor Author

I just realized the tests were checking the error output for some reason. I switched it to normal output, so now all tests pass.

@KAAtheWiseGit KAAtheWiseGit marked this pull request as ready for review October 28, 2023 12:31
fdncred
fdncred approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

Thanks. Seems to be working well from my testing.

@fdncred fdncred added the pr:release-note-mention Addition/Improvement to be mentioned in the release notes label Oct 30, 2023
@fdncred fdncred merged commit 72f7b9b into nushell:main Oct 30, 2023
20 checks passed
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
@KAAtheWiseGit @hardfau1t
Add umkdir command (nushell#10785)
1c11c37 
A `mkdir` command, which uses `uu_mkdir` as backend.

close nushell#10515.
@fdncred fdncred mentioned this pull request Jan 16, 2024
Open
16 tasks
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
@KAAtheWiseGit @dmatos2012
Add umkdir command (nushell#10785)
505c982 
A `mkdir` command, which uses `uu_mkdir` as backend.

close nushell#10515.
@hustcer hustcer mentioned this pull request Feb 28, 2024
Merged
fdncred pushed a commit that referenced this pull request Feb 28, 2024
@hustcer
Making coreutils umkdir as the default mkdir (#12007)
7b95e37 
<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->
`umkdir` was added in #10785, I think it's time to replace the default
one.

# After Submitting

Remove the old `mkdir` command and making coreutils' `umkdir` as the
default
@sarubo sarubo mentioned this pull request Mar 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdncred fdncred fdncred approved these changes

Assignees
No one assigned
Labels
coreutils-uutils Changes relating to coreutils/uutils file-system Related to commands and core nushell behavior around the file system pr:release-note-mention Addition/Improvement to be mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add uutils/coreutils mkdir command into nushell
4 participants
@KAAtheWiseGit @amtoine @fdncred @sholderbach