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

Logger: skip ANSI color commands if output is not a tty #744

Merged
merged 1 commit into from
Nov 30, 2016

Conversation

aviddiviner
Copy link
Contributor

Here's a little improvement to the default logger so that the output isn't colorised if we're not outputting to a terminal. So, if we run with ./server | tee my.log then instead of my log file looking like this:

[GIN] 2016/11/17 - 16:28:44 |^[[97;42m 200 ^[[0m|   24.459651ms | ::1 |^[[97;44m  ^[[0m GET     /
[GIN] 2016/11/17 - 16:28:44 |^[[97;42m 200 ^[[0m|  1.766299995s | ::1 |^[[97;44m  ^[[0m GET     /ws
[GIN] 2016/11/17 - 16:28:44 |^[[97;42m 200 ^[[0m|   11.265361ms | ::1 |^[[97;44m  ^[[0m GET     /static/favicon.gif

It looks like this:

[GIN] 2016/11/17 - 16:28:44 | 200 |   24.459651ms | ::1 |   GET     /
[GIN] 2016/11/17 - 16:28:44 | 200 |  1.766299995s | ::1 |   GET     /ws
[GIN] 2016/11/17 - 16:28:44 | 200 |   11.265361ms | ::1 |   GET     /static/favicon.gif

The same applies to any pipe. You can see this in action by just running ./server | cat

This does add an extra dependency on golang.org/x/crypto/ssh/terminal but it's pretty widely used and stable. I did consider copying over the IsTerminal function, but there's a bit of tricky syscall magic there and I'd rather they take care of it.

The other change I could think you might want to do, is assume that it's not a terminal if it isn't a valid *os.File, so change line 50 to:

isTerm := false

That might be nice for people funnelling their logs off to other random io.Writers that don't want the ANSI code spam. But I've just kinda left it as it is for now.

Let me know what you think! Thanks 😄

@coveralls
Copy link

coveralls commented Nov 17, 2016

Coverage Status

Coverage increased (+0.02%) to 94.107% when pulling f263a35 on aviddiviner:logger-fix into bb159f9 on gin-gonic:master.

@appleboy
Copy link
Member

@aviddiviner Open your pull request against develop not master

@aviddiviner aviddiviner changed the base branch from master to develop November 18, 2016 11:39
@codecov-io
Copy link

codecov-io commented Nov 18, 2016

Current coverage is 93.08% (diff: 100%)

Merging #744 into develop will increase coverage by 0.02%

@@            develop       #744   diff @@
==========================================
  Files            15         15          
  Lines          1268       1273     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1180       1185     +5   
  Misses           76         76          
  Partials         12         12          

Powered by Codecov. Last update 9177f01...70ada0c

@aviddiviner
Copy link
Contributor Author

@appleboy Sure, done.

@aviddiviner
Copy link
Contributor Author

Any news on this?

@appleboy
Copy link
Member

LGTM

@tboerger
Copy link
Contributor

Also LGTM

@appleboy appleboy merged commit c3bfd69 into gin-gonic:develop Nov 30, 2016
@appleboy
Copy link
Member

@aviddiviner Thanks.

javierprovecho added a commit that referenced this pull request Dec 3, 2016
This reverts commit c3bfd69, reversing
changes made to 9177f01.
javierprovecho added a commit that referenced this pull request Dec 3, 2016
This reverts commit c3bfd69, reversing
changes made to 9177f01.
javierprovecho added a commit that referenced this pull request Dec 3, 2016
@appleboy
Copy link
Member

appleboy commented Dec 4, 2016

@aviddiviner we reverted this PR since Google App Engine can't working. Please refer to #752. We need to try another way to resolve this issue.

appleboy pushed a commit that referenced this pull request Feb 1, 2017
* Revert "Merge pull request #753 from gin-gonic/bug"

This reverts commit 556287f, reversing
changes made to 32cab50.

* Revert "Merge pull request #744 from aviddiviner/logger-fix"

This reverts commit c3bfd69, reversing
changes made to 9177f01.

* Update gin version
appleboy pushed a commit that referenced this pull request May 29, 2017
* Revert "Merge pull request #753 from gin-gonic/bug"

This reverts commit 556287f, reversing
changes made to 32cab50.

* Revert "Merge pull request #744 from aviddiviner/logger-fix"

This reverts commit c3bfd69, reversing
changes made to 9177f01.

* add custom Delims support

* add some test for Delims

* remove the empty line for import native package

* remove unuseful comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants