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

Don't assume connection to a tty when number of columns is set #975

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goth-turtle
Copy link

This fixes the colorization issue from #955. Im not sure about introducing an unsafe block for that libc call though, can I get an opinion on that?

@ariasuni
Copy link
Collaborator

ariasuni commented Nov 4, 2021

I think the unsafe block is fine, but I don’t think this call is portable e.g. on Windows which we want to support in the future.

I think that we should leverage the fact that the terminal_size crate is cross-platform and that the function terminal_size::terminal_size() returns None if there’s no TTY, and it could be extracted from src/output/mod.rs and used in main.rs.

@goth-turtle
Copy link
Author

Im not sure if I understood you correctly, but for now I just replaced the isatty() call with terminal_size().

I was also wondering if adding a testcase for this bug would be helpful? Ive looked into that already, but I dont think I entirely understand how to add a test yet, so I would need some help if you think I should add one.

@ariasuni
Copy link
Collaborator

ariasuni commented Dec 5, 2021

I suppose it’s something that you could test with Vagrant. See https://github.com/ogham/exa#testing-with-vagrant. Then it’s possible to add a test where exa is piped. Let me know if you want to do this or if you have questions.

I could do it but as you may have noticed by the time I took to comment your PR, I don’t have much time for exa these days, so it would be cool if you take a stab at it.

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.

2 participants