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

Added interface to allow different printing styles. #34

Closed
wants to merge 2 commits into from

Conversation

iansmith
Copy link

This change is a no-op for existing goblin users. This is only
useful if you are running in a terminal that doesn't understand
the ansi color codes. This matters to users of go sublime because
sublime isnt a terminal so the colors and check make the output
hard to read.

This change is a no-op for existing goblin users.  This is only
useful if you are running in a terminal that doesn't understand
the ansi color codes. This matters to users of go sublime because
sublime isnt a terminal so the colors and check make the output
hard to read.
@marcosnils
Copy link
Member

Nice change!.

I'll take a look once I get home!.

@iansmith
Copy link
Author

@marcosnils Thanks.

@marcosnils
Copy link
Member

Ian, i've seen the change and I have a couple of questions

  1. Is it possible to update the PR with the correct indentations?. I see you use 2 spaces but everything else in goblin is coded with 4 spaces.

  2. I see that in goblin.go you're initializing the TerminalFancier always. How and when is the TextFancier being used?. Can you provide a quick example?

It'd would be great to auto-detect if goblin is running under a tty and then disable colors automatically. Mocha actually does it so I think it won't be difficult to achieve.

@xetorthio what do you think?

Thanks,

Marcos.

@iansmith
Copy link
Author

  1. I use gofmt always for code formatting... not sure what else to tell you about the issue of spaces.

  2. I've updated the code to detect a terminal (good idea!) and then pick the appropriate fancier.

@marcosnils
Copy link
Member

@iansmith things look way better now.

Just one more detail before merging. Check that Travis marked this merge as unstable as it didn't pass the integration build. For what I've seen you need to add the terminal dependency into the makefile in order for the tests to run.

Please update your pull request so we can merge your changes.

Thanks,

Marcos.

@marcosnils
Copy link
Member

Closed.

Please continue un #35

@marcosnils marcosnils closed this Nov 25, 2013
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

2 participants