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

feat: Listing files only #381

Merged
merged 6 commits into from
Sep 23, 2023
Merged

Conversation

hulxv
Copy link
Contributor

@hulxv hulxv commented Sep 18, 2023

What's new?

Add '-f' and '--only-files' flag to list files only (like '-D' and '--only-dirs' to list dirs only)

Example:

content

  hello.rs 
  bye.rs 
  todo.txt
  README.md
  tests/
  docs/ 
  projects/
 

usage

$ eza --only-files -1
hello.rs 
bye.rs 
todo.txt
README.md

Close #367

@hulxv hulxv requested a review from cafkafk as a code owner September 18, 2023 21:17
src/fs/filter.rs Outdated
}
(false, true) => {
// On pass -'-only-files' flag only
files.retain(File::is_file)
Copy link
Member

Choose a reason for hiding this comment

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

Add a semicolon for consistent formatting. Like this:

files.retain(File::is_file);

@9glenda
Copy link
Member

9glenda commented Sep 18, 2023

@hulxv try running cargo clippy next time before opening a pull request. Nothing wrong with your code but the CI fails if cargo clippy warnings are in the code.

src/fs/filter.rs Outdated
@@ -41,6 +41,9 @@ pub struct FileFilter {
/// Whether to only show directories.
pub only_dirs: bool,

/// Whether to only show files.
pub only_files: bool,
Copy link
Member

Choose a reason for hiding this comment

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

According to clippy

error: more than 3 bools in a struct
--> src/fs/filter.rs:27:1
|
27 | / pub struct FileFilter {
28 | |
29 | | /// Whether directories should be listed first, and other types of file
30 | | /// second. Some users prefer it like this.
... |
65 | | pub git_ignore: GitIgnore,
66 | | }
| |_^
|
= help: consider using a state machine or refactoring bools into two-variant enums
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#struct_excessive_bools
= note: -D clippy::struct-excessive-bools implied by -D warnings

error: could not compile eza (bin "eza") due to previous error
Error: Process completed with exit code 101.

Copy link
Member

@9glenda 9glenda Sep 18, 2023

Choose a reason for hiding this comment

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

I think this is not that big of a problem but I'm not really into rust types so maybe it should be addressed.

But as far as I understand it this can't be done through and enums as clippy suggests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I working on this. some minutes and I will push the solution

@hulxv
Copy link
Contributor Author

hulxv commented Sep 18, 2023

I think this is the best way to solve the clippy error

Copy link
Member

@9glenda 9glenda left a comment

Choose a reason for hiding this comment

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

LGTM

/// Whether to only show directories.
pub only_dirs: bool,
// Flags that the file filtering process follow
pub flags: Vec<FileFilterFlags>,
Copy link
Member

Choose a reason for hiding this comment

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

Nice adding a vector and using an enum. Yes I think this is way more elegant then the struct.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Thanks, this a nice improvement, and the code looks good. Thanks for also adding a trycmd test.

Could you add completions for:

  • zsh
  • fish
  • bash

Also, you'd have to document your changes in:

  • the manpage man/eza.1.md
  • the README.md

Comment on lines +85 to +94
match (self.flags.contains(&OnlyDirs), self.flags.contains(&OnlyFiles)) {
(true, false) => {
// On pass -'-only-dirs' flag only
files.retain(File::is_directory);
}
(false, true) => {
// On pass -'-only-files' flag only
files.retain(File::is_file);
}
_ => {}
Copy link
Member

Choose a reason for hiding this comment

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

Doing it like this means that -f --only-dirs falls through and doesn't just show no files. It's probably the best solution, because -f --only-dirs is completely useless if it didn't show anything.

@hulxv
Copy link
Contributor Author

hulxv commented Sep 20, 2023

I'm not familiar with bash. I read completions/bash/eza but I didn't understand what I should do to add completion for --only-files

@cafkafk
Copy link
Member

cafkafk commented Sep 20, 2023

I'm not familiar with bash. I read completions/bash/eza but I didn't understand what I should do to add completion for --only-files

Honestly, it is fine if you don't wanna do the bash completions, few if any here know how to do them and they're horribly out of date anyways. With no bash users as contributors we can skip it tbh.

@daviessm
Copy link
Contributor

daviessm commented Sep 20, 2023

With no bash users as contributors we can skip it tbh.

I'm an unfortunate bash user (historical reasons), I'll try to go through all the completions and make a PR to fix them all at some point.

Edit: #400

@cafkafk
Copy link
Member

cafkafk commented Sep 20, 2023

With no bash users as contributors we can skip it tbh.

I'm an unfortunate bash user (historical reasons), I'll try to go through all the completions and make a PR to fix them all at some point.

Ohh, well, thanks for looking into it! You're both the designated windows and bash guy now :p

@hulxv
Copy link
Contributor Author

hulxv commented Sep 20, 2023

Is all done or is there still anything should I do?

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Is all done or is there still anything should I do?

Nope, seems good to me. Tested sandboxed integrations tests, tested behavior locally, tested completions still work for zsh and fish. Also checked man page generation still works and looks good.

Thanks for the contribution, merging in a moment 👍

@hulxv
Copy link
Contributor Author

hulxv commented Sep 23, 2023

This PR hasn't merged yet. Is there any problem?

@cafkafk
Copy link
Member

cafkafk commented Sep 23, 2023

This PR hasn't merged yet. Is there any problem?

Yes... I forgot it. Thanks for pinging me >_<

@cafkafk cafkafk merged commit c5b544c into eza-community:main Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

feat: Listing files only
4 participants