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

use ansi-to-html to get colored output #9

Merged
merged 4 commits into from
May 24, 2014

Conversation

mmmries
Copy link
Contributor

@mmmries mmmries commented May 20, 2014

This addresses issue #8

This is my very first attempt to contributing anything to atom (actually my first attempt to do anything with node.js at all) so all feedback is welcome. The current test suite runs for me, but the spec about the resize handle is failing on the current master branch.

I didn't write any tests for my code since I literally have no idea what I am doing, but if you are inclined to merge this I would be happy to go back and be more thorough.

This is what the output looks like for me right now:

screen shot 2014-05-19 at 9 45 37 pm

@moxley
Copy link
Owner

moxley commented May 20, 2014

Thanks for the pull request. That's really cool!

I wish it would show some colors for me, but right now, it only shows the medium gray over the theme's dark background. When there's time, I'll play around with it and see if the color situation can be improved.

ansi-output
.

@mmmries
Copy link
Contributor Author

mmmries commented May 20, 2014

One thing I did run into when I was testing with it last night was that it
didn't seem to pick up the .rspec file in the root of my project (which is
usually where I tell it to output colors), but if you configure the colors
inside your test file (like with minitest or via RSpec.configure) then the
colors came through just fine.

We should probably also make the default text color in our pane a little
brighter because that gray makes it hard to read the text.

On Tue, May 20, 2014 at 9:08 AM, Moxley Stratton
notifications@github.comwrote:

Thanks for the pull request. That's really cool!

I wish it would show some colors for me, but right now, it only shows the
medium gray over the theme's dark background. When there's time, I'll play
around with it and see if the color situation can be improved.

[image: ansi-output]https://cloud.githubusercontent.com/assets/86996/3028936/4f0337e4-e030-11e3-948e-d1fb1853277a.png
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-43638651
.

@calmyournerves
Copy link
Contributor

I wanted to implement this too. But after seeing no colors and reading nodejs/node-v0.x-archive#2754 I eventually gave up.

I just tried your branch and saw no colors at first. Adding config.color_enabled = true to RSpec.configure didn't help either.
But after adding config.tty = true I finally got colors!

@calmyournerves
Copy link
Contributor

FYI: Adding --tty on the command line works for me too.

@@ -13,7 +14,7 @@ class RubyTestView extends View
@span outlet: 'header'
@div class: "panel-body", =>
@div class: 'ruby-test-spinner', 'Starting...'
@pre "", outlet: 'results'
@div "", outlet: 'results'
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a pre tag renders just fine and is indented too (and the text is brighter for me too). I'd revert this.

@moxley
Copy link
Owner

moxley commented May 20, 2014

@calmyournerves I tried your suggestion with RSpec.configure { |c| c.tty = true }, and the colors showed up. However, that probably breaks other consumers (e.g., TravisCI) who really don't want ANSI output.

Maybe there is something that can be passed to the running process to fool it into thinking it's being run from a terminal? Something like this: http://stackoverflow.com/questions/1401002/trick-an-application-into-thinking-its-stdin-is-interactive-not-a-pipe.

@calmyournerves
Copy link
Contributor

@moxley I tried https://github.com/chjj/tty.js last time and it showed colors, but it seems like a pretty complicated solution for this problem.
As I have mentioned, adding --tty to rspec on the command line also works, I simply added it to the "Rspec command" setting in Atom.

@moxley
Copy link
Owner

moxley commented May 20, 2014

@calmyournerves: Yeah, tty.js looks like a full terminal emulator.

I use zeus to start up my Rails environment in the background, then zeus rspec ./path/to/file_spec.rb to run the spec file. It doesn't pass any rspec options (like --tty) back to rspec. I could add config.tty = true if $stdout.isatty to spec_helper.rb, but it requires everybody in my situation to do the same thing. I'll investigate the script command and see if it will do the trick. I suspect there is a way to do it.

@mmmries
Copy link
Contributor Author

mmmries commented May 20, 2014

Basically all of my projects (open source and at work) specify config.color
= true in the spec_helper.rb. It seems odd that rspec is doing all of this
magical checking of the environment to decide when to use colors and when
not to, but I suppose if we can find a way that makes it work more
consistently for more use cases we should probably pursue it.

On Tue, May 20, 2014 at 3:40 PM, Moxley Stratton
notifications@github.comwrote:

@calmyournerves https://github.com/calmyournerves: Yeah, tty.js looks
like a full terminal emulator.

I use zeus to start up my Rails environment in the background, then zeus
rspec ./path/to/file_spec.rb to run the spec file. It doesn't pass any
rspec options (like --tty) back to rspec. I could add config.tty = true
if $stdout.isatty to spec_helper.rb, but it requires everybody in my
situation to do the same thing. I'll investigate the script command and
see if it will do the trick. I suspect there is a way to do it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-43686943
.

@mmmries
Copy link
Contributor Author

mmmries commented May 20, 2014

@calmyournerves I switched the div back to a pre and it does look a lot better. If we make the default rspec commands just include --tty then we cover all the cases except where someone is using zeus (or some other background loader) right? Is that good enough coverage for now? Maybe add a section to the README for helps if colors don't show up?

@moxley
Copy link
Owner

moxley commented May 20, 2014

Yeah @hqmq, it seems strange that Rspec doesn't blindly take what you specify for the --color or config.color option and output ANSI based on that. There's probably a good reason for it though. Reading the source code, they call IO#tty? (on $stdout, I think) to determine whether the consumer is attached to a terminal.

Any way, adding documentation will work for now. But eventually, I'd like for users to not have to add special configuration to get colors they're already used to seeing. It's possible. I prepended the test command with the script command and that caused the test output to render in color, although there were some problems associated with it.

Totally on a different subject, this PR introduces a new problem. The output is left unescaped, so special HTML characters like < get interpreted as HTML artifacts. For example, the text #<Nokogiri::XML::Element:0x3fdefa33eb54 name="hr"> becomes #. Makes it impossible to read some types of debugging data.

@mmmries
Copy link
Contributor Author

mmmries commented May 20, 2014

oh, that is interesting. The ansi-to-html library emits html, so I had to switch to using innerHTML rather than escaping it, I wonder if there is a way to get the ansi-to-html library to escape html characters as it is processing the data stream

@moxley
Copy link
Owner

moxley commented May 20, 2014

Now that I think about it, that sounds like a bug in ansi-to-html.

@mmmries
Copy link
Contributor Author

mmmries commented May 22, 2014

I've got a pull request into ansi-to-html, once that gets merged and released we should be able to just pickup the new package version and run with it.

@mmmries
Copy link
Contributor Author

mmmries commented May 24, 2014

@moxley ansi-to-html released an update that included my escaping code. I've integrated it here. Perhaps I should write a specific test case for it? I wasn't sure, since it is really the responsibility of ansi-to-html.

@moxley
Copy link
Owner

moxley commented May 24, 2014

Looks great. I don't think a test is necessary for this. It's a visual enhancement rather than a functional one. And I agree about the HTML escaping bit-- since it's part of ansi-to-html, we shouldn't test it.

moxley added a commit that referenced this pull request May 24, 2014
use ansi-to-html to get colored output
@moxley moxley merged commit 17d0e85 into moxley:master May 24, 2014
@moxley
Copy link
Owner

moxley commented Jun 6, 2014

It turns out zeus does pass options on to rspec, or at least it does with "--tty" and "--color". So, no more requiring tweaks to spec_helper.rb.

Also, I'm going to change the default rspec command and add "--tty --color" to the command.

@mmmries
Copy link
Contributor Author

mmmries commented Jun 7, 2014

Probably also useful to note that as of RSpec 3.0 there is an HTML
formatter that is part of rspec-core. So perhaps once RSpec 3.x becomes the
more widely used version we might want to just invoke it with the HTML
formatter as the default rather than --tty --color and having to html escape

On Fri, Jun 6, 2014 at 5:55 PM, Moxley Stratton notifications@github.com
wrote:

It turns out zeus does pass options on to rspec, or at least it does with
"--tty" and "--color". So, no more requiring tweaks to spec_helper.rb.

Also, I'm going to change the default rspec command and add "--tty
--color" to the command.


Reply to this email directly or view it on GitHub
#9 (comment).

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

3 participants