-
-
Couldn't load subscription status.
- Fork 338
feat: add --spacing parameter #1558
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your contribution. This is indeed a great idea and a really useful one to clarify output. I made some comments feel free to discuss them if you want. I will add two more general ones, first if you could cleanup the commit history so that we can understand more what happenned. I dont think you need to squash everything into one but maybe separate into the different parties. Also for the tests, the one you wrote yourself need to have a stdout and stderr file I believe (please tell me if I am wrong). And you will need to generate and update the powertest, for this I would recommend installing https://github.com/eza-community/powertest and using our Justfile. If you have any questions or need help feel free to ask
@MartinFillon Edit: rewrote the whole commit history for a better understanding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats some great work you did there. I dont have any complaints based on the code. Now as I said you still need to update the ptests. If you need help on how to as my previous comment might not be the best I can provide some. You will also need to sort out the CI not passing, I would recommend for that to use nix but if you have trouble please ask for help !
I'm having several issues trying to run Unfortunately, I don't have time to keep debugging this. Could you help me with that? |
|
|
||
| </div> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some changing is unrelated to this PR feat: add --spacing parameter
we'd better submit those commit in a separate PR
| Check out the themes available in the official [eza-themes](https://github.com/eza-community/eza-themes) repository, or contribute your own. | ||
|
|
||
| An example theme file is available in `docs/theme.yml`, and needs to either be placed in a directory specified by the | ||
| An example theme file is available in `docs/theme.yml`, and needs to either be placed in a directory specified by the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
|
||
| If you wanna contribute to eza, firstly, you're expected to follow our | ||
| [code of conduct](https://github.com/eza-community/eza/blob/main/CODE_OF_CONDUCT.md). | ||
| If you wanna contribute to eza, firstly, you're expected to follow our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| --treat-dirs-as-files(-d) # List directories like regular files | ||
| --level(-L): string # Limit the depth of recursion | ||
| --width(-w) # Limits column output of grid, 0 implies auto-width | ||
| --spacing # Space between columns in grid mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| --spacing # Space between columns in grid mode | |
| --spacing # Space between columns in grid mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review, haven't read all of the code (i.e., there will likely be a more detailed review of that later).
I think before anything my primary concern here is that it seems even cargo test fails according to CI.
| complete -c eza -l git-ignore -d "Ignore files mentioned in '.gitignore'" | ||
| complete -c eza -s a -l all -d "Show hidden and 'dot' files. Use this twice to also show the '.' and '..' directories" | ||
| complete -c eza -s A -l almost-all -d "Equivalent to --all; included for compatibility with `ls -A`" | ||
| complete -c eza -s A -l almost-all -d "Equivalent to --all; included for compatibility with $(ls -A)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand why the $() is nescesarry here?
| {-f,--only-files}"[List only files]" \ | ||
| {-L,--level}"+[Limit the depth of recursion]" \ | ||
| {-w,--width}"+[Limits column output of grid, 0 implies auto-width]" \ | ||
| --spacing"+[Set the space between columns]" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remember the zsh syntax but this does seem odd compared to other lines, why not --spacing"[Set ...]" \?
| `-w`, `--width=COLS` | ||
| : Set screen width in columns. | ||
|
|
||
| `--spacing=COLS` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COLS should be something appropriate for the context, e.g. spacing
| pub static CLASSIFY: Arg = Arg { short: Some(b'F'), long: "classify", takes_value: TakesValue::Optional(Some(WHEN), "auto") }; | ||
| pub static DEREF_LINKS: Arg = Arg { short: Some(b'X'), long: "dereference", takes_value: TakesValue::Forbidden }; | ||
| pub static WIDTH: Arg = Arg { short: Some(b'w'), long: "width", takes_value: TakesValue::Necessary(None) }; | ||
| pub static SPACING:Arg = Arg { short: None, long: "spacing", takes_value: TakesValue::Necessary(None) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be formatted like adjacent lines
| --no-user suppress the user field | ||
| --no-time suppress the time field | ||
| --stdin read file names from stdin, one per line or other separator | ||
| --stdin read file names from stdin, one per line or other separator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem like it should be introduced in this pr
This adds a --spacing parameter to the cli. This is related to #520.
Initially I thought of adding the parameter as --space-between-columns, but the name seemed to be too long to be typed all the time.
Simple
Before:

After:

Long
Before:

After:
