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

Logging changes #39

Closed
wants to merge 2 commits into from
Closed

Logging changes #39

wants to merge 2 commits into from

Conversation

astubbs
Copy link
Contributor

@astubbs astubbs commented Feb 28, 2014

No description provided.

@mtedone
Copy link
Owner

mtedone commented Feb 28, 2014

I don't intend to swap log4j for slf4j

@astubbs
Copy link
Contributor Author

astubbs commented Feb 28, 2014

Why?

@astubbs
Copy link
Contributor Author

astubbs commented Feb 28, 2014

Are you familiar with slf4j?

@mtedone
Copy link
Owner

mtedone commented Mar 1, 2014

I could decide to swap to slf4j if we used it as a wrapper for log4j but
not as a standalone replacement for log4j.

On Fri, Feb 28, 2014 at 9:50 PM, Antony Stubbs notifications@github.comwrote:

Are you familiar with slf4j?

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

@astubbs
Copy link
Contributor Author

astubbs commented Mar 1, 2014

Podam is a library - it should not dictate what logging implementation is used in compile scope by it's client code.

But of course you can use sld4j-log4j in test scope if you like.

On 1/03/2014, at 5:14 am, Marco Tedone notifications@github.com wrote:

I could decide to swap to slf4j if we used it as a wrapper for log4j but
not as a standalone replacement for log4j.

On Fri, Feb 28, 2014 at 9:50 PM, Antony Stubbs notifications@github.comwrote:

Are you familiar with slf4j?

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


Reply to this email directly or view it on GitHub.

@mtedone
Copy link
Owner

mtedone commented Mar 1, 2014

Sorry but I can't make sense of what you're saying. Whether log4j or slf4j,
one logging implementation is required and I've chosen log4j. Now if you
want to use slf4j, please amend your pull request to use sld4j-log4j in the
dependency definition and then I'll be OK to switch to the slf4j facade.

On Sat, Mar 1, 2014 at 1:01 PM, Antony Stubbs notifications@github.comwrote:

Podam is a library - it should not dictate what logging implementation is
used in compile scope by it's client code.

But of course you can use sld4j-log4j in test scope if you like.

On 1/03/2014, at 5:14 am, Marco Tedone notifications@github.com
wrote:

I could decide to swap to slf4j if we used it as a wrapper for log4j but
not as a standalone replacement for log4j.

On Fri, Feb 28, 2014 at 9:50 PM, Antony Stubbs notifications@github.comwrote:

Are you familiar with slf4j?

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36398088>
.

Reply to this email directly or view it on GitHub.

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

@astubbs
Copy link
Contributor Author

astubbs commented Mar 1, 2014

slf4j isnt' a logging implementation, it's just that - a facade.

Being a library - the point with slf4j is that you aren't' dictating what
logging implementation your users must use. I.e. if they're using logback,
then all the podam logging gets pipped through slf4j into a logback bridge.
If they're using log4j, it goes through the log4j bridge.

By directly including the log4j bridge, or using log4j directly, your
enforcing the choice of logger.

On 1 March 2014 16:14, Marco Tedone notifications@github.com wrote:

Sorry but I can't make sense of what you're saying. Whether log4j or slf4j,
one logging implementation is required and I've chosen log4j. Now if you
want to use slf4j, please amend your pull request to use sld4j-log4j in the
dependency definition and then I'll be OK to switch to the slf4j facade.

On Sat, Mar 1, 2014 at 1:01 PM, Antony Stubbs <notifications@github.com

wrote:

Podam is a library - it should not dictate what logging implementation is
used in compile scope by it's client code.

But of course you can use sld4j-log4j in test scope if you like.

On 1/03/2014, at 5:14 am, Marco Tedone notifications@github.com
wrote:

I could decide to swap to slf4j if we used it as a wrapper for log4j
but
not as a standalone replacement for log4j.

On Fri, Feb 28, 2014 at 9:50 PM, Antony Stubbs <
notifications@github.com>wrote:

Are you familiar with slf4j?

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36398088>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36424106>

.

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

@mtedone
Copy link
Owner

mtedone commented Mar 2, 2014

Afaik slf4j has also got its own implementation. To use a backed logging
framework you need to include the right jar and I guess you need to choose
the right Logger (unless it does it for you). What I'm saying is that for
PODAM the underlying logger should be log4j and if we agree on this the
facade could be slf4j.

On Saturday, March 1, 2014, Antony Stubbs notifications@github.com wrote:

slf4j isnt' a logging implementation, it's just that - a facade.

Being a library - the point with slf4j is that you aren't' dictating what
logging implementation your users must use. I.e. if they're using logback,
then all the podam logging gets pipped through slf4j into a logback bridge.
If they're using log4j, it goes through the log4j bridge.

By directly including the log4j bridge, or using log4j directly, your
enforcing the choice of logger.

On 1 March 2014 16:14, Marco Tedone <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');>
wrote:

Sorry but I can't make sense of what you're saying. Whether log4j or
slf4j,
one logging implementation is required and I've chosen log4j. Now if you
want to use slf4j, please amend your pull request to use sld4j-log4j in
the
dependency definition and then I'll be OK to switch to the slf4j facade.

On Sat, Mar 1, 2014 at 1:01 PM, Antony Stubbs <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');

wrote:

Podam is a library - it should not dictate what logging implementation
is
used in compile scope by it's client code.

But of course you can use sld4j-log4j in test scope if you like.

On 1/03/2014, at 5:14 am, Marco Tedone <notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');

wrote:

I could decide to swap to slf4j if we used it as a wrapper for log4j
but
not as a standalone replacement for log4j.

On Fri, Feb 28, 2014 at 9:50 PM, Antony Stubbs <
notifications@github.comjavascript:_e(%7B%7D,'cvml','notifications@github.com');
wrote:

Are you familiar with slf4j?

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36398088>
.

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36424106>

.

Reply to this email directly or view it on GitHub<
https://github.com/mtedone/podam/pull/39#issuecomment-36436879>
.

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

@astubbs
Copy link
Contributor Author

astubbs commented Mar 3, 2014

slf4j has a couple, but none are used by default. I've adjusted the swap commit to include the slf4j-log4j12 bridge (which beings in log4j for the implementation).
I've also added the string concatenation guards.

Protect debug statements from unnecessary string concatenation.
@astubbs
Copy link
Contributor Author

astubbs commented Apr 15, 2014

mtedone can you please comment on this?

@astubbs
Copy link
Contributor Author

astubbs commented Apr 15, 2014

Using {} circumvents the issue with building the String before checking the debug level, so there's no performance impact- you don't need the isdebug guard.

@daivanov
Copy link
Collaborator

This cannot be merged any more and was addressed in other commits.

@daivanov daivanov closed this Jan 18, 2015
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