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

mkdir: make mkdir public and document it #5329

Merged
merged 1 commit into from
Oct 10, 2023
Merged

mkdir: make mkdir public and document it #5329

merged 1 commit into from
Oct 10, 2023

Conversation

KAAtheWiseGit
Copy link
Contributor

@KAAtheWiseGit KAAtheWiseGit commented Sep 27, 2023

  • Made the mkdir function public. Added documentation, describing the behavior and parameters
  • Moved the GNU dot-stripping behavior from exec to mkdir.

@KAAtheWiseGit KAAtheWiseGit marked this pull request as draft September 27, 2023 17:54
@KAAtheWiseGit
Copy link
Contributor Author

@tertsdiepraam, opened the PR. It doesn't have much as of now, since I haven't made mkdir public.

///
/// ## Options
///
/// * `recurisve` --- create parent directories for the given dirs, if they do
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// * `recurisve` --- create parent directories for the given dirs, if they do
/// * `recursive` --- create parent directories for the given dirs, if they do

@tertsdiepraam
Copy link
Member

Thanks for opening the PR! For context for others, we were talking here: #5321.


A little recap:

The question is: what should be exposed? exec or mkdir?

  • exec: makes multiple directories and some GNU behaviour fix.
  • mkdir: makes a single directory.

Multiple directories is usually not good for nushell, because they what control over the errors and make them pretty with miette. So, we prefer not to print errors but to return them (with Result). Another question is whether the GNU behaviour is expected by nushell.


With that out of the way, here are my thoughts. I think it makes sense to expose a function for making a single directory and let nushell do the looping for good errors. I also think the GNU fix at least doesn't hurt for nushell, so I see no reason to exclude it.

So, in my opinion, the GNU fix is in the wrong spot: it should be in mkdir and then we can expose that.

- Made the `mkdir` function public.  Added documentation, describing the
  behavior and parameters
- Moved the GNU dot-stripping behavior from `exec` to `mkdir`.
@KAAtheWiseGit KAAtheWiseGit changed the title mkdir: make exec public and document it mkdir: make mkdir public and document it Sep 27, 2023
@KAAtheWiseGit
Copy link
Contributor Author

Rewrote the commit:

  • Made mkdir public instead of exec.
  • Completed the documentation GNU behavior note.
  • Moved the GNU patch into mkdir.

Changed the PR name/description appropriately.

@KAAtheWiseGit KAAtheWiseGit marked this pull request as ready for review October 9, 2023 15:53
@KAAtheWiseGit
Copy link
Contributor Author

I think the PR is ready for review.

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I won't merge this myself because I'm reviewing from the GH app, but this looks great!

/// ## Trailing dot
///
/// To match the GNU behavior, a path with the last directory being a single dot
/// (like `some/path/to/.`) is created (with the dot stripped).
Copy link
Member

Choose a reason for hiding this comment

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

Great comment here!

@cakebaker cakebaker merged commit c70a47f into uutils:main Oct 10, 2023
45 checks passed
@cakebaker
Copy link
Contributor

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants