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

Colorized output #10

Merged
merged 6 commits into from Sep 24, 2018
Merged

Colorized output #10

merged 6 commits into from Sep 24, 2018

Conversation

@paderinandrey
Copy link
Contributor

@paderinandrey paderinandrey commented Sep 20, 2018

Added color output in console https://cultofmartians.com/tasks/logux-rails-log.html
Also moved the logic of logging in a separate class

@@ -66,10 +69,6 @@ def self.process_batch(stream:, batch:)
Logux::Process::Batch.new(stream: stream, batch: batch).call
end

def self.logger
Copy link
Member

@wilddima wilddima Sep 21, 2018

Why did you decide to remove this method? It looks more straightforward to use just logger method. Also, your approach a little bit harder to maintain, in the case when we want to replace the logger with external implementation(in that case, we should replace it on every call, instead of just in one method). I think better to replace the content of this method with a link to your class.

Copy link
Contributor Author

@paderinandrey paderinandrey Sep 21, 2018

Yes, you're right, it's better. I fixed it

# frozen_string_literal: true

module Logux
class Logger
Copy link
Member

@wilddima wilddima Sep 21, 2018

I don't see reasons to use class here. Maybe module will be better.

Copy link
Member

@wilddima wilddima left a comment

Hi @paderinandrey
Thanks for your pull-request, it looks quite good.
I left some comments with a hope that we can make a perfect implementation. 🙂

# frozen_string_literal: true

module Logux
module Logging
Copy link
Member

@wilddima wilddima Sep 24, 2018

Let's revert it back to Logger name. I don't see reasons to use Logging, instead of familiar Logger.

logger.info(msg)
end

def self.error(message)
Copy link
Member

@wilddima wilddima Sep 24, 2018

If we aren't changing the default behavior of this methods, will be better to use delegate from ActionSupport.

logger.error(message)
end

def self.warn(message)
Copy link
Member

@wilddima wilddima Sep 24, 2018

The same here

Copy link
Member

@wilddima wilddima left a comment

Hi @paderinandrey
Thank for changes, and it looks quite better.
I again left some comments.

I test it in our app, and right now it looks that way:
https://cl.ly/e6271c1edfd1
Maybe, I have a strange term config, but anyway. Let's remove the blue background, and make it more colorless. Also let's wrap in bold messages: Receive from Logux:, Write to logux: and so on.

@paderinandrey
Copy link
Contributor Author

@paderinandrey paderinandrey commented Sep 24, 2018

Hi @wilddima
Yes it depends on the term settings. Now I made it explicit.

@wilddima
Copy link
Member

@wilddima wilddima commented Sep 24, 2018

I think it's good enough for the current version
@paderinandrey thank you for your work and patience! 👍

@wilddima wilddima merged commit b6424a7 into logux:master Sep 24, 2018
1 check passed
@wilddima wilddima mentioned this pull request Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants