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

upmerge #170 - logging interface extension for debug logging #197

Closed
wants to merge 13 commits into from

Conversation

gbirchmeier
Copy link
Member

Merged @ligu's #170 submission to master

@gbirchmeier
Copy link
Member Author

Hey, @ligu - your fix is really really good. I did make some slight changes and add some documentation. I still need to do some manual testing. (I also renamed "Debug" config to "ExtraDebugLogging".)

I'd appreciate it if you'd have a look at my additions and welcome any comments you might have.

@roji
Copy link

roji commented May 21, 2013

Sorry to jump in, but out of curiosity, is there a reasons for not using one of the standard logging libraries (e.g. NLog, log4net), or better yet, Commons.Logging as an abstraction layer to allow plugging into whatever library the embedding application happens to use?

In my case we use NLog, and I've had to implement a bridge from QuickFixN ILog to NLog, it was trivial but seems unnecessary?

@gbirchmeier
Copy link
Member Author

@roji - We don't use another log library simply because no one has implemented it. I'm not opposed to it.

@roji
Copy link

roji commented May 23, 2013

@gbirchmeier and @ligu, I can devote a little bit of time next week to integrating Common.Logging (https://github.com/net-commons/common-logging) for the event/debug logs (not sure if the incoming/outgoing logging should be changed as well, what do you think?).

This would probably roll back/modify some of the work in this pull request. Specifically, ILog would no longer have OnEvent() and OnDebug() methods at all, since these would go to Common.Logging instead.

Do you guys think this is a good idea?

@gbirchmeier
Copy link
Member Author

@roji - I think you are proposing a good thing. Can you open a new issue for us to discuss it?

@gbirchmeier
Copy link
Member Author

Actually, @roji, wouldn't it just make sense to create a CommonLoggerLogFactory? Then people can choose to use Common.Logging or stay with the existing loggers. You wouldn't need to rewrite anything.

@ligu
Copy link
Contributor

ligu commented Jun 7, 2013

Altough I've put a lot of effort to extend logging functionality based on the "traditional" logging infrastructure, I agree with @roji that it would worth the work to overthrow the old logging with a well designed Common.Logging solution.

And if decision has been made, I suggest the complete change: logging of messages, events, debug, everything to the new logging framework, total removal of old logging from everywhere, no "half work"!
Sure, this would be a new major version...

I absolutele encourage to continue the work started in #205 !

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ gbirchmeier
❌ ligu
You have signed the CLA already but the status is still pending? Let us recheck it.

@gbirchmeier
Copy link
Member Author

Closing for staleness. Keep an eye on the QF/n mailing list though, I do want to open discussion about logging revisions.

@gbirchmeier gbirchmeier closed this Feb 9, 2024
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.

4 participants