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: add quotations around filenames with spaces. exa pr#1165 #318

Merged
merged 4 commits into from
Sep 27, 2023

Conversation

PThorpe92
Copy link
Member

#311

the commit history was a mess on the original PR, and its time to revisit this :D

image

feedback/testers needed

@PThorpe92 PThorpe92 requested a review from a team September 14, 2023 14:37
@PThorpe92 PThorpe92 linked an issue Sep 14, 2023 that may be closed by this pull request
@cafkafk cafkafk added this to the v1.0 milestone Sep 14, 2023
@cafkafk
Copy link
Member

cafkafk commented Sep 15, 2023

Wait, are we doing this before #195, not sure if we came to any conclusions on element.

@PThorpe92
Copy link
Member Author

Wait, are we doing this before #195, not sure if we came to any conclusions on element.

This came about from the referenced issue + digging up a couple of the remaining exa PR's.. I don't remember this one being the one we were waiting on clap for, but that is perfectly fine with me either way as I think it's a good 1.0 addition.

@gierens
Copy link
Member

gierens commented Sep 16, 2023

Just tested this locally, looks solid 👍 ... but I agree we should wait for clap and the properly review this.

@ariasuni
Copy link
Contributor

ariasuni commented Sep 16, 2023

I think quotes should use a different color when they’re not part of the filename but added by exa, so that we can see immediately that they’re not part of the filename

Edit: For immediate and future reference, my take on the quoting styles we would like and they should work:
ogham/exa#216 (comment)

@gierens
Copy link
Member

gierens commented Sep 17, 2023

I think quotes should use a different color when they’re not part of the filename but added by exa, so that we can see immediately that they’re not part of the filename

Uh that's ah really nice idea :o :)

@cafkafk
Copy link
Member

cafkafk commented Sep 19, 2023

Maybe we shouldn't block this until clap is done, given how big of a task that turned out to be?

src/options/help.rs Outdated Show resolved Hide resolved
src/options/file_name.rs Outdated Show resolved Hide resolved
src/options/file_name.rs Outdated Show resolved Hide resolved
Copy link
Member

@gierens gierens left a comment

Choose a reason for hiding this comment

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

LGTM :) ... just marked a few formatting nitpicks :D

@PThorpe92
Copy link
Member Author

@gierens those lines were all left over from stripping the other functionality from the PR and fixing merge conflicts, just to give some context. fixing now :D

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.

Can you add completions for zsh and fish, and add it to the manpage. You don't have to do bash because #400 will probably do it.

@sevz17
Copy link
Contributor

sevz17 commented Sep 22, 2023

I think quotes should use a different color when they’re not part of the filename but added by exa, so that we can see immediately that they’re not part of the filename

That's a good idea, but what happens when you use --color=never?

@gierens
Copy link
Member

gierens commented Sep 22, 2023

@gierens those lines were all left over from stripping the other functionality from the PR and fixing merge conflicts, just to give some context. fixing now :D

Yeah makes total sense ... I mean without rustfmt such things are bound to happen ... that's why I made the PR for that. :D

@ariasuni
Copy link
Contributor

I think quotes should use a different color when they’re not part of the filename but added by exa, so that we can see immediately that they’re not part of the filename

That's a good idea, but what happens when you use --color=never?

ls align files differently, I think it’s a great idea but not sure how easy it is to do it in our current code base.

$ ls -l
total 52
drwxr-xr-x 15 ariasuni ariasuni  4096  6 sept. 16:10  administratif/
drwxr-xr-x  9 ariasuni ariasuni  4096 17 sept. 01:09  documents/
drwxr-xr-x 12 ariasuni ariasuni  4096  6 juil. 13:33  images/
drwxr-xr-x 21 ariasuni ariasuni  4096 28 août  01:33  jeux/
drwxr-xr-x  6 ariasuni ariasuni  4096 22 sept. 18:58  musique/
drwxr-xr-x 11 ariasuni ariasuni  4096 17 juil.  2021  projets/
drwxr-xr-x  5 ariasuni ariasuni  4096 22 sept. 20:14  téléchargements/
-rw-r--r--  1 ariasuni ariasuni     0 22 sept. 23:06 'test file'
drwxr-xr-x  6 ariasuni ariasuni 20480 22 sept. 20:14  vidéos/

cafkafk
cafkafk previously approved these changes Sep 23, 2023
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.

Sandboxed integration tests pass, and this looks good to me when locally testing.

2023-09-23_06-03
The --no-quotes flag also works
2023-09-23_06-03_1

Fish and zsh completions also work, and the man pages are also correct. Nice change, LGTM 👍

@cafkafk
Copy link
Member

cafkafk commented Sep 23, 2023

I think it’s a great idea but not sure how easy it is to do it in our current code base.

Could we do a hacky \r prefixed to the first quote? I'm not entirely sure how the rendering code works, or if we wanna actually change the grid code to do this.

@gierens gierens self-requested a review September 23, 2023 18:09
gierens
gierens previously approved these changes Sep 23, 2023
@PThorpe92 PThorpe92 dismissed stale reviews from gierens and cafkafk via 4496892 September 24, 2023 15:26
@PThorpe92
Copy link
Member Author

Had to fix merge conflicts with main.. idk why tests didn't pass. seems to still work fine locally. I'll look at this deeper

@MartinFillon
Copy link
Contributor

For the unit tests action your code is not formatted and as for nix the code doesn't follow nix flake check guidelines.

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.

Just checked out the changes, still seem good to me.

@cafkafk cafkafk merged commit 51790ed into main Sep 27, 2023
15 checks passed
@cafkafk cafkafk deleted the p-quote_filenames branch September 27, 2023 01:26
@eggbean
Copy link

eggbean commented Oct 11, 2023

I know ls does this, but what is the practical reason for it? It just looks messier to me.

@gierens
Copy link
Member

gierens commented Oct 11, 2023

I know ls does this, but what is the practical reason for it? It just looks messier to me.

There are several cases where this can help ... to actually see trailing spaces, to directly copy paste those filenames into further commands without having to add the quotes manually, and of course also for piping eza output into other things ...

If you don't want to see the quotes to can use the --no-quotes option.

@aarondill

This comment was marked as duplicate.

@aarondill
Copy link

Sorry for the ping. I didn't see #515 or #547.

@DenebTM
Copy link

DenebTM commented Dec 4, 2023

oh cool, my PR finally did get merged, without credit but alas :3

I know ls does this, but what is the practical reason for it? It just looks messier to me.

My motivation was: KDE's Konsole relies on the quotes for shell integration stuff, like hovering, CTRL+clicking, or highlighting file names by double click.

@PThorpe92
Copy link
Member Author

oh cool, my PR finally did get merged, without credit but alas :3

Yeah looks like that is my fault, I think the original PR cafk moved over from exa had ping'd you as the author, but that ended up sitting around forever on an old branch so I had to fix so many conflicts and whatnot that I put in a new PR, and only put the link ( exa PR#1165 ) in the name.

My apologies, and thank you for the contribution ❤️

@John-Gee
Copy link

John-Gee commented Dec 14, 2023

I think the different color for eza quotes from @ariasuni is a great idea, cause as it is it's hard to see at a glance that it was added by eza and not part of the name. Maybe using the same color but more pale or darker? Since it's mostly a technicality it does not need to be as visible as the rest.

Also is it possible to have the same type of quotes for all? For instance if a folder uses single quotes in its filename eza will add double quote for that folder but will still use single quotes for the rest, it's kind of messy.

Quotes also break alignment I'd say, is it possible to add a spacing to those that don't need quotes, or maybe someone has a better idea for that? Adding quotes to all? Not sure.

@cafkafk
Copy link
Member

cafkafk commented Dec 15, 2023

@John-Gee It would probably be a good idea to open an issue or discussion for this, that way your feedback wont be lost!

@John-Gee
Copy link

@cafkafk very well, done with #728 thank you!

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: show special chars as shown by ls