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.

Sign up for GitHub

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

Jump to bottom

(exa PR) 908: Don’t show icons when piped (also add --icons={always,auto,never}) #54

Closed
wants to merge 3 commits into from
Closed

(exa PR) 908: Don’t show icons when piped (also add --icons={always,auto,never}) #54

wants to merge 3 commits into from

Conversation

cafkafk
Copy link
Member

@cafkafk cafkafk commented Jul 29, 2023 •
edited
Loading

https:/github.com/ogham/exa/pull/908

@cafkafk cafkafk changed the title (exa PR) 908 (exa PR) 908: Don’t show icons when piped (also add --icons={always,auto,never}) Jul 29, 2023
@cafkafk cafkafk added the not ready for PRs that aren't finished label Jul 29, 2023
@cafkafk cafkafk marked this pull request as draft July 30, 2023 04:20
@sbatial sbatial mentioned this pull request Jul 30, 2023
Closed
63 tasks
@cafkafk cafkafk added this to the exa pulls done milestone Jul 31, 2023
@github-actions
Copy link

This pull request is stale because it has been open for 30 days with no activity.

@PThorpe92
Copy link
Member

hmm. Moving to 1.70 stabilizes is_terminal() correct?

This could use revisiting after the move for sure, but i'm fixing it now to see whats up with it.

@ariasuni
Copy link
Contributor

@PThorpe92 I don’t understand why you removed the always option, it’s useful when you want to show icons when piped, similar to the --colour option.

@PThorpe92
Copy link
Member

PThorpe92 commented Sep 26, 2023 •
edited
Loading

I'm sorry you are right, it's very possible that I fixed that locally. This just shows the initial fix of the merge conflicts here. I worked on it a bunch locally but couldn't get the dont show in TTY | piped option working in all cases. I really need to look at it again i've just been super busy.

Ok I had just enough time to clean this up and tinker with it a little, but I don't think it's ready yet. I'll have more time this weekend. @ariasuni if you want to review and let me know if anything sticks out to you.

@PThorpe92 PThorpe92 requested a review from ariasuni September 26, 2023 17:10
@PThorpe92 PThorpe92 removed the not ready for PRs that aren't finished label Oct 12, 2023
@PThorpe92
Copy link
Member

PThorpe92 commented Oct 12, 2023 •
edited
Loading

image

should close #527 and i think at least one more.

I think this should be all set now. The only thing is this will break everyone's alias's because the always, auto, never are traditionally a necessary argument flag.

@PThorpe92 PThorpe92 linked an issue Oct 12, 2023 that may be closed by this pull request
feat: Use is-terminal crate to mimic exact ls output even with flags when being piped. #527
Closed
@PThorpe92 PThorpe92 marked this pull request as ready for review October 12, 2023 21:21
@PThorpe92 PThorpe92 requested a review from gierens October 12, 2023 21:26
@cafkafk
Copy link
Member Author

cafkafk commented Oct 13, 2023

I think this should be all set now. The only thing is this will break everyone's alias's because the always, auto, never are traditionally a necessary argument flag.

Could we have it so --icons by itself implied --icons=auto? That way we're still almost backwards compat, and I think it's probably the most sensible option... What do you think?

cafkafk
cafkafk commented Oct 13, 2023
View reviewed changes
Copy link
Member Author

@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.

We probably need to update bash, zsh, fish, and nushell completions, as well as the manual and README.md

@cafkafk
Copy link
Member Author

cafkafk commented Oct 13, 2023

Could you potentially reopen this as a PR by you instead, I can't make suggestions like normally on my own PR :s

@PThorpe92
Copy link
Member

Oh yup. gotta do the docs and completions still. Yeah it was wierd trying to request ur review on ur own PR :P I'll re open one

@PThorpe92
Copy link
Member

Closing to open a new one.. I added more functionality

@PThorpe92 PThorpe92 closed this Oct 16, 2023
@PThorpe92 PThorpe92 deleted the pr-908 branch October 16, 2023 13:49
@PThorpe92 PThorpe92 mentioned this pull request Oct 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ariasuni ariasuni Awaiting requested review from ariasuni

@gierens gierens Awaiting requested review from gierens gierens is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Eza Pull-Requests
Status: ✅ Done
Milestone
exa pulls
Development

Successfully merging this pull request may close these issues.

feat: Use is-terminal crate to mimic exact ls output even with flags when being piped.
3 participants
@cafkafk @PThorpe92 @ariasuni