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

Fix ANSI colors in Windows CMD and PowerShell #28

Merged
merged 5 commits into from Feb 20, 2019
Merged

Fix ANSI colors in Windows CMD and PowerShell #28

merged 5 commits into from Feb 20, 2019

Conversation

Phundrak
Copy link

@Phundrak Phundrak commented Feb 14, 2019

Hi there!

This is a fix for the issue #15. Turns out CMD and PowerShell both support ANSI colors, but it is disabled by default and it can be enabled with the correct system calls. And that’s basically what the crate output_vt100 does, which I’ve just released.

I also made the output_vt100::init() function (which activates the escaped characters for Windows) to be called at launch for any crate calling pretty_assertions thanks to the crate ctor, but maybe it should be made optional? (since it apparently also fails the Rust 1.17 tests)

@colin-kiegel
Copy link
Collaborator

@Phundrak seems like we have to bump the minimum Rust version with those new dependencies. Can you tell me the lowest Rust version this still compiles with? Thx.

@colin-kiegel
Copy link
Collaborator

Oh and is it possible to pull those dependencies only, when compiling for windows?

@colin-kiegel
Copy link
Collaborator

I had a look at your crates source code and it looks good.

However, I'm a bit worried about side-effects, if we ship it with pretty-assertions. We would be changing the terminal state of applications we don´t know... I wouldn´t expect something like that from a macro library.

Can you put it behind something like a "colors-on-windows" feature flag? Or do you have a better idea? Do you think a warning would suffice?

I´m open to suggestions. :-)

@colin-kiegel
Copy link
Collaborator

Btw, I decided that the next release 0.6.0 will target the Rust 2018 edition. So we can bump the minimum Rust Version to 1.31. :-)

@nbouteme
Copy link
Contributor

It's true that at the end of the day, it's kind of a hack, but the alternative requires an extensive rewrite, and seeing as Microsoft will eventually enable VT100 emulation by default in CMD and Powershell, will require to revert back all the changes, while this could just sit behind a flag for Windows targets until Microsoft finalize their support for VT100, and could be then removed.
About the terminal state, I think it's safe to assume any application writing to a terminal using pretty_assertions is expecting an ANSI compliant VT100.
Alternatively, the macros could just call output_vt100::init() each time they need to output since calling it multiple time has the same effects as a no-op. This would also remove the dependency on ctor.

Copy link
Collaborator

@colin-kiegel colin-kiegel left a comment

Choose a reason for hiding this comment

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

please put a short note in the documentation about possible side-effects on windows - and make the dependencies platform-specific

NOTE: your changes compile with Rust 1.31, which is good enough for the next scheduled release.

thx :-)

@@ -18,3 +18,5 @@ travis-ci = { repository = "colin-kiegel/rust-pretty-assertions" }
[dependencies]
difference = "2.0.0"
ansi_term = "0.11"
output_vt100 = "0.1"
ctor = "0.1.7"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@colin-kiegel colin-kiegel merged commit 13fa78f into rust-pretty-assertions:master Feb 20, 2019
@colin-kiegel
Copy link
Collaborator

v0.6.0 is out 🎉

Thanks alot for ur contribution

@RazrFalcon
Copy link

RazrFalcon commented Apr 26, 2020

Alternatively, the macros could just call output_vt100::init() each time they need to output since calling it multiple time has the same effects as a no-op. This would also remove the dependency on ctor.

Would it be possible to implement? ctor is a very heavy dependency due to procedural macros. Adn when creating a vendored archive it pulls the winapi crate, which is like 100MiB.

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.

None yet

4 participants