LOG4NET-586 provide log4j like XML Layout that works on .NET Core 1.x #22
Conversation
|
There is a lot of whitespace changes in this PR which makes it tough to review. Would you mind to apply those changes onto develop and only keep the changes/features in this PR? |
src/Layout/XmlLayoutBaseNS.cs
Outdated
| /// <summary> | ||
| /// The namespace URI to use for the elements and attributes written by this layout. | ||
| /// </summary> | ||
| public virtual string NamespaceUri |
There was a problem hiding this comment.
Do we need this property with a backing field to be virtual?
There was a problem hiding this comment.
I've decided to override the getters rather than setting default values in ActivateOptions or the constructor.
There was a problem hiding this comment.
As far as I can tell, ActivateOptions is in existing appenders or layouts the place where configurations are applied. Other parts should not provide mechanisms to customize behavior because that means that ActivateOptions is no longer the source of the one and only truth.
There was a problem hiding this comment.
it's about providing a default value, not really customizing anything. I'd move it to the constructor and make the properties non-virtual unless you prefer to get rid of the base class completely.
src/Layout/XmlLayoutBaseNS.cs
Outdated
| /// </para> | ||
| /// </remarks> | ||
| public virtual string Prefix | ||
| { |
There was a problem hiding this comment.
Do we need this to be virtual?
| /// </para> | ||
| /// </remarks> | ||
| abstract public class XmlLayoutBaseNS : LayoutSkeleton | ||
| { |
There was a problem hiding this comment.
Would you please explain the reasoning behind this base class? As far as I can tell we could reduce both the inheritance complexity and the public api without it. I discourage inheritance and encourage composition when there is need for it.
There was a problem hiding this comment.
Initially I planned to also create a replacement of XmlLayout as it suffers from the same problems if you specify a prefix. I'm not sure this is something people need anymore, in which case we can merge the classes.
There was a problem hiding this comment.
Does this indicate that we should rather implement a XmlFormatter class that can be plugged into various appenders? This is the composition what I have in mind and that we should prefer over inheritance.
There was a problem hiding this comment.
The layout is what gets plugged into the appenders, isn't it? The whole structure of XmlLayoutBaseNS and XmlLayoutSchemaLog4jNS just mimics what has been there before - an area of code I never really looked into so far.
| /// handle XML using namespaces. | ||
| /// </para> | ||
| /// </remarks> | ||
| /// <author>Nicko Cadell</author> |
There was a problem hiding this comment.
but he's written most of the code I copied :-)
| @@ -28,7 +28,7 @@ | |||
| namespace log4net.Layout | |||
| { | |||
| /// <summary> | |||
| /// Layout that formats the log events as XML elements compatible with the log4j schema | |||
| /// Layout that formats the log events as XML elements compatible with the log4j 1.2 schema | |||
There was a problem hiding this comment.
The newly added layout class claims to do this, too. Would you please explain this change?
There was a problem hiding this comment.
the class has a version field for historical reasons but only accepts 1.2 as value. To me it is more important to say it is the 1.x version and not the 2.x version of log4j.
There was a problem hiding this comment.
If that's the case, fine with me.
| /// </para> | ||
| /// </remarks> | ||
| /// <author>Nicko Cadell</author> | ||
| public class XmlLayoutSchemaLog4jNS : XmlLayoutBaseNS |
There was a problem hiding this comment.
Would we like to have the version in the class name, too? If tomorrow there is to be 1.3 layout we have no place to put that.
There was a problem hiding this comment.
2.0 rather than 1.3 - I wanted to keep symmetry with the existing class name.
There was a problem hiding this comment.
So here we could actually really make use of a XmlFormatter class that provides mechanisms to format logging events in various formats. To mention a few we're already having today:
- log4j_1.2
- log4j_1.2_with_namespace
This could be provided by one and only one layout (XmlLayoutSchemaLog4j?) which in turn can be configured to either be in format 1.2 or 1.2 with namespaces etc.. This would also create a place for future 2.0 layouts to live in. What do you think?
There was a problem hiding this comment.
The responsibility of the layout is formatting, I don't really see what an additional formatter would do. Please note that we need to configure XmlWriter in different ways for the "no namespace" and "with namespace" cases.
|
The whitespace changes are inside a separate commit - ec82f5b - I'll cherry-pick it to develop and rebase this branch. |
asfgit
force-pushed
the
feature/LOG4NET-586
branch
from
ec82f5b
to
cc8a057
Compare
asfgit
force-pushed
the
feature/LOG4NET-586
branch
from
9dc4926
to
4da2ae2
Compare
|
Good day It's been quite a while since this PR last saw activity. In an attempt to get pull requests under some semblance of This is not because the contributions aren't valuable - it's simply a matter of being the only person spending I encourage you to re-submit the PR against the current main branch if the issue is still significant. Your |
No description provided.