Skip to content

Conversation

amadaluzia
Copy link
Contributor

What does this PR change?

This PR reimplements the shadowed ls alias to use a do { |closure| } expression instead of making two custom commands alongside an alias. (Though, it could be rewritten into a custom command with the loss of documentation of the original command.)

Why?

I thought the manner the docs achieved an ls shadow could be changed because 2 custom commands seemed like a fair bit.

@fdncred
Copy link
Contributor

fdncred commented Jan 31, 2025

This is the syntax I've adopted. Yours seems to drop the LS_COLORS highlighting.

# List the filenames, sizes, and modification times of items in a directory.
def lss [
    --all (-a),         # Show hidden files
    --long (-l),        # Get all available columns for each entry (slower; columns are platform-dependent)
    --short-names (-s), # Only print the file names, and not the path
    --full-paths (-f),  # display paths as absolute paths
    --du (-d),          # Display the apparent directory size ("disk usage") in place of the directory metadata size
    --directory (-D),   # List the specified directory itself instead of its contents
    --mime-type (-m),   # Show mime-type in type column instead of 'file' (based on filenames only; files' contents are not examined)
    --threads (-t),     # Use multiple threads to list contents. Output will be non-deterministic.
    ...pattern: glob,   # The glob pattern to use.
]: [ nothing -> table ] {
    let pattern = if ($pattern | is-empty) { [ '.' ] } else { $pattern }
    (ls
        --all=$all
        --long=$long
        --short-names=$short_names
        --full-paths=$full_paths
        --du=$du
        --directory=$directory
        --mime-type=$mime_type
        --threads=$threads
        ...$pattern
    ) | sort-by type name -i
}

# alias the built-in ls command to ls-builtins
alias ls-builtin = ls

# alias lss to the new ls custom command
alias ls = lss

@amadaluzia
Copy link
Contributor Author

That's a solid improvement over mine I'll say, though I think this would be better:

alias ls-builtin = ls

# List the filenames, sizes, and modification times of items in a directory.
def ls [
    --all (-a),         # Show hidden files
    --long (-l),        # Get all available columns for each entry (slower; columns are platform-dependent)
    --short-names (-s), # Only print the file names, and not the path
    --full-paths (-f),  # display paths as absolute paths
    --du (-d),          # Display the apparent directory size ("disk usage") in place of the directory metadata size
    --directory (-D),   # List the specified directory itself instead of its contents
    --mime-type (-m),   # Show mime-type in type column instead of 'file' (based on filenames only; files' contents are not examined)
    --threads (-t),     # Use multiple threads to list contents. Output will be non-deterministic.
    ...pattern: glob,   # The glob pattern to use.
]: [ nothing -> table ] {
    let pattern = if ($pattern | is-empty) { [ '.' ] } else { $pattern }
    (ls
        --all=$all
        --long=$long
        --short-names=$short_names
        --full-paths=$full_paths
        --du=$du
        --directory=$directory
        --mime-type=$mime_type
        --threads=$threads
        ...$pattern
    ) | sort-by type name -i
}

@fdncred
Copy link
Contributor

fdncred commented Jan 31, 2025

I get this when trying to run yours.

 ls
Error: nu::shell::recursion_limit_reached

  × Recursion limit (50) reached
     ╭─[C:\Users\us991808\AppData\Roaming\nushell\scripts\defs.nu:233:25]
 232          ...pattern: glob,   # The glob pattern to use.
 233  ╭─▶ ]: [ nothing -> table ] {
 234         let pattern = if ($pattern | is-empty) { [ '.' ] } else { $pattern }
 235         (ls
 236             --all=$all
 237             --long=$long
 238             --short-names=$short_names
 239             --full-paths=$full_paths
 240             --du=$du
 241             --directory=$directory
 242             --mime-type=$mime_type
 243             --threads=$threads
 244             ...$pattern
 245         ) | sort-by type name -i
 246  ├─▶ }
     · ╰──── This called itself too many times
 247 
     ╰────

Also, these aliases should have comments for providing help.

# alias the built-in ls command to ls-builtins
alias ls-builtin = ls

# alias lss to the new ls custom command
alias ls = lss

@amadaluzia
Copy link
Contributor Author

amadaluzia commented Jan 31, 2025

I get this when trying to run yours.
...
Error: nu::shell::recursion_limit_reached

× Recursion limit (50) reached

Oh, I forgot something.

# alias the built-in ls command to ls-builtins
alias ls-builtin = ls

# List the filenames, sizes, and modification times of items in a directory.
def ls [
    --all (-a),         # Show hidden files
    --long (-l),        # Get all available columns for each entry (slower; columns are platform-dependent)
    --short-names (-s), # Only print the file names, and not the path
    --full-paths (-f),  # display paths as absolute paths
    --du (-d),          # Display the apparent directory size ("disk usage") in place of the directory metadata size
    --directory (-D),   # List the specified directory itself instead of its contents
    --mime-type (-m),   # Show mime-type in type column instead of 'file' (based on filenames only; files' contents are not examined)
    --threads (-t),     # Use multiple threads to list contents. Output will be non-deterministic.
    ...pattern: glob,   # The glob pattern to use.
]: [ nothing -> table ] {
    let pattern = if ($pattern | is-empty) { [ '.' ] } else { $pattern }
    (ls-builtin
        --all=$all
        --long=$long
        --short-names=$short_names
        --full-paths=$full_paths
        --du=$du
        --directory=$directory
        --mime-type=$mime_type
        --threads=$threads
        ...$pattern
    ) | sort-by type name -i
}

@fdncred
Copy link
Contributor

fdncred commented Jan 31, 2025

Yup, that works better.

@fdncred
Copy link
Contributor

fdncred commented Feb 1, 2025

If you can update this PR to reflect the discussion, we can land it.

@amadaluzia
Copy link
Contributor Author

Alright, I'll get to that then.

@amadaluzia amadaluzia force-pushed the refactor-nushell-ls-alias branch from d1707d0 to 5f15096 Compare February 1, 2025 14:26
@amadaluzia
Copy link
Contributor Author

@fdncred It has been corrected.

@amadaluzia amadaluzia force-pushed the refactor-nushell-ls-alias branch from 5f15096 to ee3ebcf Compare February 1, 2025 16:24
@fdncred fdncred merged commit e94eabc into nushell:main Feb 1, 2025
2 checks passed
@fdncred
Copy link
Contributor

fdncred commented Feb 1, 2025

Thanks

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 this pull request may close these issues.

3 participants