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

Allow custom loggers #768

Merged
merged 12 commits into from Jun 2, 2017
Merged

Allow custom loggers #768

merged 12 commits into from Jun 2, 2017

Conversation

binarylogic
Copy link
Contributor

@binarylogic binarylogic commented Apr 11, 2017

Currently Hanami only takes an array of arguments that get passed to the Hanami::Logger class. This change allows the use of a custom Logger by passing a single object during configuration. For example:

Hanami.configure do
  environment :production do
    logger Timber::Logger.new(STDOUT)
  end
end

This change was prompted in order to install the Timber logger which handles capturing context and structure automatically.

Copy link
Member

@davydovanton davydovanton left a comment

Choose a reason for hiding this comment

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

I like this idea. But I have some little issues and what to discuss about it.

@hanami/core WDYT?

@@ -28,7 +28,13 @@ module Components # rubocop:disable Metrics/ModuleLength
end

resolve do |configuration|
Hanami::Logger.new(Hanami.environment.project_name, *configuration.logger) unless configuration.logger.nil?
if configuration.logger.is_a?(Array)
if configuration.logger.length == 1 && configuration.logger[0].is_a?(::Logger)
Copy link
Member

Choose a reason for hiding this comment

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

maybe just configuration.logger.fist.is_a?(::Logger)?

Copy link
Member

Choose a reason for hiding this comment

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

and are you sure that this code will work with other loggers (for example logging)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I don't see any reason it wouldn't unless the internal usage of the logger in Hanami deviates from the standard ::Logger interface. Otherwise, this should work great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe just configuration.logger.fist.is_a?(::Logger)?

Done

@binarylogic
Copy link
Contributor Author

binarylogic commented Apr 12, 2017

Yes, please let me know the consensus. Hanami is so refreshing to work with in comparison to alternatives, so it's no surprise we have multiple customers at @timberio that are using it. But they can't unless we patch how the logger is set. Moreover, there are quite a few other Logging alternatives (as you pointed out), and restricting everyone to ::Logger feels too opinionated in my opinion.

@binarylogic
Copy link
Contributor Author

Also, do you mind expanding on the issues that concern you? Maybe I can help or write code that addresses them?

Copy link
Member

@mereghost mereghost left a comment

Choose a reason for hiding this comment

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

Albeit not being a fan of double ifs, I have no suggestion to refactor that, so.... LGTM

@binarylogic
Copy link
Contributor Author

binarylogic commented Apr 17, 2017

Thanks! Are we good to ship it?

@mereghost
Copy link
Member

@davydovanton any other non-addressed points?

@mereghost mereghost changed the base branch from master to develop April 17, 2017 16:04
@jodosha jodosha added this to the v1.1.0 milestone Apr 25, 2017
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

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

@binarylogic Hi and thanks for this PR. LGTM 👍

Side note: I don't like that if, but it seems the easiest way to implement this feature so :shipit:.

@binarylogic Would you be so kind to add the documentation part of this on hanami/hanami.github.io repository, so it will appear here: http://hanamirb.org/guides/projects/logging/

Tnx! 👍

@jodosha
Copy link
Member

jodosha commented Apr 25, 2017

@davydovanton Please review again an let us to know WDYT. Thanks.

@jodosha jodosha mentioned this pull request Apr 25, 2017
Copy link
Member

@davydovanton davydovanton left a comment

Choose a reason for hiding this comment

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

great work 👏

@binarylogic
Copy link
Contributor Author

Would you be so kind to add the documentation part of this on hanami/hanami.github.io repository, so it will appear here:

@jodosha absolutely, I'll try to get on it today or tomorrow

@mereghost
Copy link
Member

mereghost commented Apr 25, 2017

As everyone is happy and merry, @binarylogic could you rebase it on hanami:develop and push it, so I can merge it?

@jodosha
Copy link
Member

jodosha commented Apr 25, 2017

@mereghost by clicking on "Squash and merge" should solve the rebase problem, right?

@mereghost
Copy link
Member

@jodosha It seems it's not an interactive squash, so I can't discard commits. >.<

@binarylogic
Copy link
Contributor Author

Hey guys, any update here? It appears the branch has no conflicts at the moment.

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.

None yet

4 participants