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

No-op non-windows enable_ansi_support function #30

Open
daboross opened this issue Sep 4, 2017 · 9 comments · May be fixed by rustadopt/ansiterm-rs#10
Open

No-op non-windows enable_ansi_support function #30

daboross opened this issue Sep 4, 2017 · 9 comments · May be fixed by rustadopt/ansiterm-rs#10
Assignees

Comments

@daboross
Copy link

daboross commented Sep 4, 2017

Hi!

This looks like an awesome crate to use for colours. One question though: could a non-windows version of enable_ansi_support be added to the crate?

Ideally I'd rather not have my CLI applications having any OS-specific code - it would be nice if I could just stick ansi_term::enable_ansi_support() at the top of my main() function, and know that it'll do whatever's needed for the current OS.

It could just be literally #[cfg(not(windows))] fn enable_ansi_support() -> Result<(), u64> { Ok(()) }. If you'd be willing to add this, I can also submit a PR.

@H2CO3
Copy link

H2CO3 commented Sep 4, 2017

I think it would be even better to have the library call the enable_ansi_support() function automagically if it's on Windows. That way there's no separate initialization code that the developer should remember to call and that can, consequently, get out of sync / forgotten / etc, and the "enabling" process is completely hidden from the user of the library, shrinking the API surface and decreasing complexity.

@daboross
Copy link
Author

daboross commented Sep 5, 2017

@H2CO3 Where in the library would this be located - and how would the library make it obvious it's doing a semi-expensive and environment affecting operation then?

There'd have to be some sort of global Once to ensure that it doesn't happen twice, and locking to support that... Would you want this to happen inside the paint method, which is otherwise very cheap - literally just a stack value construction?

I think it makes sense to have some 'prep' code - it can be performed by whatever library or application plans on using color codes - most likely a logging library of one sort or another.

@H2CO3
Copy link

H2CO3 commented Sep 5, 2017

@daboross I think it should not be in paint. After all, that method doesn't actually do anything. It should be in a place of which the performance is comparable to the performance of locking. It could be in the <ANSIString as Display>::fmt() method itself, for example.

@daboross
Copy link
Author

daboross commented Sep 6, 2017

@H2CO3 Ah ok, that's reasonable.

I'd still be a bit hesitant, since someone could be formatting to a string for later storage or printing from another program, but that'd be better than in paint. It seems a bit 'magic' still.

@H2CO3
Copy link

H2CO3 commented Sep 6, 2017

@daboross I guess at this point it's really a matter of points of view :) In this particular case I would prefer the "magic" (ie. hiding the complexity) because it's not a particularly performance-sensitive issue. In other situations, I sometimes prefer being explicit too. In any case, it would be good to make the library platform-independent and, if a function has to be called sometimes, then provide an empty implementation for those OSes that don't need it.

@ogham ogham self-assigned this Sep 22, 2017
@joshtriplett
Copy link
Collaborator

I'd prefer to have the no-op enable_ansi_support function and call that at startup; calling it implicitly from fmt seems entirely too magic, and as @daboross noted, you might not be writing to the console.

@zackfall
Copy link

zackfall commented Jun 6, 2021

This is not implemented yet?

@sunshowers
Copy link

I wanted enable_ansi_support without the rest of ansi_term, so I've included the no-op function and published it as a separate crate (repository).

@xTachyon
Copy link

+1 that enable_ansi_support should be a no-op on any OS that is not windows.

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 a pull request may close this issue.

7 participants