FreeAndNil commented on code in PR #126:
URL: https://github.com/apache/logging-log4net/pull/126#discussion_r1528410182
##########
src/log4net/Core/LoggingEvent.cs:
##########
@@ -840,44 +800,41 @@ private string TryGetCurrentUserName()
}
}
- private string _cachedWindowsIdentityUserName;
+ private string? _cachedWindowsIdentityUserName;
private static string TryReadWindowsIdentityUserName()
{
using var identity = WindowsIdentity.GetCurrent();
- return identity?.Name ?? "";
+ return identity?.Name ?? string.Empty;
}
private static bool _platformDoesNotSupportWindowsIdentity;
/// <summary>
/// Gets the identity of the current thread principal.
/// </summary>
- /// <value>
- /// The string name of the identity of the current thread principal.
- /// </value>
/// <remarks>
/// <para>
/// Calls <c>System.Threading.Thread.CurrentPrincipal.Identity.Name</c> to
get
/// the name of the current thread principal.
/// </para>
/// </remarks>
- public string Identity
+ public string? Identity
{
get
{
- if (m_data.Identity == null && this.m_cacheUpdatable)
+ if (m_data.Identity is null && m_cacheUpdatable)
{
try
{
- if (System.Threading.Thread.CurrentPrincipal != null &&
- System.Threading.Thread.CurrentPrincipal.Identity != null &&
- System.Threading.Thread.CurrentPrincipal.Identity.Name != null)
+ if (System.Threading.Thread.CurrentPrincipal is not null &&
+ System.Threading.Thread.CurrentPrincipal.Identity is not null
&&
+ System.Threading.Thread.CurrentPrincipal.Identity.Name is not
null)
Review Comment:
```suggestion
System.Threading.Thread.CurrentPrincipal?.Identity?.Name is
string name)
```
##########
src/log4net/Core/LoggingEvent.cs:
##########
@@ -840,44 +800,41 @@ private string TryGetCurrentUserName()
}
}
- private string _cachedWindowsIdentityUserName;
+ private string? _cachedWindowsIdentityUserName;
private static string TryReadWindowsIdentityUserName()
{
using var identity = WindowsIdentity.GetCurrent();
- return identity?.Name ?? "";
+ return identity?.Name ?? string.Empty;
}
private static bool _platformDoesNotSupportWindowsIdentity;
/// <summary>
/// Gets the identity of the current thread principal.
/// </summary>
- /// <value>
- /// The string name of the identity of the current thread principal.
- /// </value>
/// <remarks>
/// <para>
/// Calls <c>System.Threading.Thread.CurrentPrincipal.Identity.Name</c> to
get
/// the name of the current thread principal.
/// </para>
/// </remarks>
- public string Identity
+ public string? Identity
{
get
{
- if (m_data.Identity == null && this.m_cacheUpdatable)
+ if (m_data.Identity is null && m_cacheUpdatable)
{
try
{
- if (System.Threading.Thread.CurrentPrincipal != null &&
- System.Threading.Thread.CurrentPrincipal.Identity != null &&
- System.Threading.Thread.CurrentPrincipal.Identity.Name != null)
+ if (System.Threading.Thread.CurrentPrincipal is not null &&
+ System.Threading.Thread.CurrentPrincipal.Identity is not null
&&
+ System.Threading.Thread.CurrentPrincipal.Identity.Name is not
null)
{
m_data.Identity =
System.Threading.Thread.CurrentPrincipal.Identity.Name;
Review Comment:
```suggestion
m_data.Identity = name;
```
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -378,33 +333,36 @@ public override ILogger GetLogger(string name)
/// </para>
/// <para>
/// The <c>Shutdown</c> method is careful to close nested
- /// appenders before closing regular appenders. This is allows
+ /// appenders before closing regular appenders. This allows
/// configurations where a regular appender is attached to a logger
/// and again to a nested appender.
/// </para>
/// </remarks>
public override void Shutdown()
{
- LogLog.Debug(declaringType, "Shutdown called on Hierarchy [" + this.Name
+ "]");
+ LogLog.Debug(declaringType, $"Shutdown called on Hierarchy [{Name}]");
// begin by closing nested appenders
Root.CloseNestedAppenders();
- lock (m_ht)
- {
- ILogger[] currentLoggers = this.GetCurrentLoggers();
+ ILogger[] currentLoggers = GetCurrentLoggers();
- foreach (Logger logger in currentLoggers)
+ foreach (ILogger logger in currentLoggers)
+ {
+ if (logger is Logger l)
{
- logger.CloseNestedAppenders();
+ l.CloseNestedAppenders();
}
+ }
- // then, remove all appenders
- Root.RemoveAllAppenders();
+ // then, remove all appenders
+ Root.RemoveAllAppenders();
- foreach (Logger logger in currentLoggers)
+ foreach (ILogger logger in currentLoggers)
Review Comment:
same as above
##########
src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs:
##########
@@ -90,70 +84,78 @@ public void Configure(XmlElement element)
if (rootElementName != CONFIGURATION_TAG)
{
- LogLog.Error(declaringType, "Xml element is - not a <" +
CONFIGURATION_TAG + "> element.");
+ LogLog.Error(declaringType, $"Xml element is - not a
<{CONFIGURATION_TAG}> element.");
return;
}
if (!LogLog.EmitInternalMessages)
{
// Look for a emitDebug attribute to enable internal debug
string emitDebugAttribute =
element.GetAttribute(EMIT_INTERNAL_DEBUG_ATTR);
- LogLog.Debug(declaringType, EMIT_INTERNAL_DEBUG_ATTR + " attribute ["
+ emitDebugAttribute + "].");
+ LogLog.Debug(declaringType, $"{EMIT_INTERNAL_DEBUG_ATTR} attribute
[{emitDebugAttribute}].");
if (emitDebugAttribute.Length > 0 && emitDebugAttribute != "null")
{
LogLog.EmitInternalMessages =
OptionConverter.ToBoolean(emitDebugAttribute, true);
}
else
{
- LogLog.Debug(declaringType, "Ignoring " + EMIT_INTERNAL_DEBUG_ATTR +
" attribute.");
+ LogLog.Debug(declaringType, $"Ignoring {EMIT_INTERNAL_DEBUG_ATTR}
attribute.");
}
}
if (!LogLog.InternalDebugging)
{
// Look for a debug attribute to enable internal debug
string debugAttribute = element.GetAttribute(INTERNAL_DEBUG_ATTR);
- LogLog.Debug(declaringType, INTERNAL_DEBUG_ATTR + " attribute [" +
debugAttribute + "].");
+ LogLog.Debug(declaringType, $"{INTERNAL_DEBUG_ATTR} attribute
[{debugAttribute}].");
if (debugAttribute.Length > 0 && debugAttribute != "null")
{
LogLog.InternalDebugging = OptionConverter.ToBoolean(debugAttribute,
true);
}
else
{
- LogLog.Debug(declaringType, "Ignoring " + INTERNAL_DEBUG_ATTR + "
attribute.");
+ LogLog.Debug(declaringType, $"Ignoring {INTERNAL_DEBUG_ATTR}
attribute.");
}
string confDebug = element.GetAttribute(CONFIG_DEBUG_ATTR);
if (confDebug.Length > 0 && confDebug != "null")
{
- LogLog.Warn(declaringType, "The \"" + CONFIG_DEBUG_ATTR + "\"
attribute is deprecated.");
- LogLog.Warn(declaringType, "Use the \"" + INTERNAL_DEBUG_ATTR + "\"
attribute instead.");
+ LogLog.Warn(declaringType, $"The \"{CONFIG_DEBUG_ATTR}\" attribute
is deprecated.");
+ LogLog.Warn(declaringType, $"Use the \"{INTERNAL_DEBUG_ATTR}\"
attribute instead.");
LogLog.InternalDebugging = OptionConverter.ToBoolean(confDebug,
true);
}
}
// Default mode is merge
- ConfigUpdateMode configUpdateMode = ConfigUpdateMode.Merge;
+ ConfigUpdateMode? configUpdateMode = ConfigUpdateMode.Merge;
// Look for the config update attribute
- string configUpdateModeAttribute =
element.GetAttribute(CONFIG_UPDATE_MODE_ATTR);
- if (configUpdateModeAttribute != null &&
configUpdateModeAttribute.Length > 0)
+ string? configUpdateModeAttribute =
element.GetAttribute(CONFIG_UPDATE_MODE_ATTR);
+ if (!string.IsNullOrEmpty(configUpdateModeAttribute))
{
// Parse the attribute
try
{
- configUpdateMode =
(ConfigUpdateMode)OptionConverter.ConvertStringTo(typeof(ConfigUpdateMode),
configUpdateModeAttribute);
+ var mode =
(ConfigUpdateMode?)OptionConverter.ConvertStringTo(typeof(ConfigUpdateMode),
configUpdateModeAttribute);
+ if (mode is not null)
Review Comment:
```suggestion
if (OptionConverter.ConvertStringTo(typeof(ConfigUpdateMode),
configUpdateModeAttribute) is ConfigUpdateMode mode)
```
##########
src/log4net/Util/PatternConverter.cs:
##########
@@ -214,9 +185,12 @@ public virtual void Format(TextWriter writer, object state)
}
}
- private static readonly string[] SPACES = { " ", " ", " ", "
", // 1,2,4,8 spaces
- " ", // 16 spaces
- " " }; // 32 spaces
+ private static readonly string[] SPACES =
+ {
+ " ", " ", " ", " ", // 1,2,4,8 spaces
+ " ", // 16 spaces
+ " "
+ }; // 32 spaces
Review Comment:
please move the comment one line up
##########
src/log4net/Util/ThreadContextStack.cs:
##########
@@ -239,7 +224,7 @@ public override string ToString()
/// </remarks>
object IFixingRequired.GetFixedObject()
Review Comment:
```suggestion
object IFixingRequired.GetFixedObject() => GetFullMessage() ??
string.Empty;
```
##########
src/log4net/Util/ThreadContextProperties.cs:
##########
@@ -116,13 +97,9 @@ public void Remove(string key)
/// Gets the keys stored in the properties.
/// </para>
/// <returns>a set of the defined keys</returns>
- public string[] GetKeys()
+ public string[]? GetKeys()
Review Comment:
```suggestion
public string[]? GetKeys() => _dictionary?.GetKeys();
```
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -378,33 +333,36 @@ public override ILogger GetLogger(string name)
/// </para>
/// <para>
/// The <c>Shutdown</c> method is careful to close nested
- /// appenders before closing regular appenders. This is allows
+ /// appenders before closing regular appenders. This allows
/// configurations where a regular appender is attached to a logger
/// and again to a nested appender.
/// </para>
/// </remarks>
public override void Shutdown()
{
- LogLog.Debug(declaringType, "Shutdown called on Hierarchy [" + this.Name
+ "]");
+ LogLog.Debug(declaringType, $"Shutdown called on Hierarchy [{Name}]");
// begin by closing nested appenders
Root.CloseNestedAppenders();
- lock (m_ht)
- {
- ILogger[] currentLoggers = this.GetCurrentLoggers();
+ ILogger[] currentLoggers = GetCurrentLoggers();
- foreach (Logger logger in currentLoggers)
+ foreach (ILogger logger in currentLoggers)
+ {
+ if (logger is Logger l)
Review Comment:
```suggestion
```
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -494,38 +454,34 @@ public override void Log(LoggingEvent logEvent)
/// The list returned is unordered but does not contain duplicates.
/// </para>
/// </remarks>
- public override Appender.IAppender[] GetAppenders()
+ public override IAppender[] GetAppenders()
{
- System.Collections.ArrayList appenderList = new
System.Collections.ArrayList();
+ var appenderList = new List<IAppender>();
CollectAppenders(appenderList, Root);
- foreach (Logger logger in GetCurrentLoggers())
+ foreach (ILogger logger in GetCurrentLoggers())
Review Comment:
same as above
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -378,33 +333,36 @@ public override ILogger GetLogger(string name)
/// </para>
/// <para>
/// The <c>Shutdown</c> method is careful to close nested
- /// appenders before closing regular appenders. This is allows
+ /// appenders before closing regular appenders. This allows
/// configurations where a regular appender is attached to a logger
/// and again to a nested appender.
/// </para>
/// </remarks>
public override void Shutdown()
{
- LogLog.Debug(declaringType, "Shutdown called on Hierarchy [" + this.Name
+ "]");
+ LogLog.Debug(declaringType, $"Shutdown called on Hierarchy [{Name}]");
// begin by closing nested appenders
Root.CloseNestedAppenders();
- lock (m_ht)
- {
- ILogger[] currentLoggers = this.GetCurrentLoggers();
+ ILogger[] currentLoggers = GetCurrentLoggers();
- foreach (Logger logger in currentLoggers)
+ foreach (ILogger logger in currentLoggers)
Review Comment:
```suggestion
foreach (Logger logger in currentLoggers.OfType<Logger>())
```
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -937,24 +849,27 @@ private static void UpdateChildren(ProvisionNode pn,
Logger log)
/// <param name="levelEntry">the level values</param>
/// <remarks>
/// <para>
- /// Define or redefine a Level using the values in the <see
cref="LevelEntry"/> argument
- /// </para>
- /// <para>
/// Supports setting levels via the configuration file.
/// </para>
/// </remarks>
internal void AddLevel(LevelEntry levelEntry)
{
- if (levelEntry == null) throw new ArgumentNullException("levelEntry");
- if (levelEntry.Name == null) throw new
ArgumentNullException("levelEntry.Name");
+ if (levelEntry is null)
+ {
+ throw new ArgumentNullException(nameof(levelEntry));
+ }
+ if (levelEntry.Name is null)
+ {
+ throw new ArgumentNullException("levelEntry.Name");
+ }
// Lookup replacement value
if (levelEntry.Value == -1)
{
- Level previousLevel = LevelMap[levelEntry.Name];
- if (previousLevel == null)
+ Level? previousLevel = LevelMap[levelEntry.Name];
+ if (previousLevel is null)
Review Comment:
```suggestion
if (LevelMap[levelEntry.Name] is Level previousLevel)
```
##########
src/log4net/Util/OptionConverter.cs:
##########
@@ -280,8 +261,8 @@ public static object ConvertTypeTo(object sourceInstance,
Type targetType)
}
// Look for a TO converter
- IConvertTo tcSource = ConverterRegistry.GetConvertTo(sourceType,
targetType);
- if (tcSource != null)
+ IConvertTo? tcSource = ConverterRegistry.GetConvertTo(sourceType,
targetType);
+ if (tcSource is not null)
Review Comment:
same as above
##########
src/log4net/Core/LoggingEvent.cs:
##########
@@ -1295,23 +1229,19 @@ protected virtual void FixVolatileData(FixFlags flags)
}
// avoid warning CS0219
- if (forceCreation != null)
+ if (forceCreation is not null)
Review Comment:
Can you please remove forceCreation in this method and replace it with _ ?
##########
src/log4net/Core/LoggingEvent.cs:
##########
@@ -1320,7 +1250,7 @@ private void CreateCompositeProperties()
{
compositeProperties.Add(logicalThreadProperties);
}
- var threadProperties = ThreadContext.Properties.GetProperties(false);
+ PropertiesDictionary threadProperties =
ThreadContext.Properties.GetProperties(false);
if (threadProperties != null)
Review Comment:
```suggestion
if (ThreadContext.Properties.GetProperties(false) is
PropertiesDictionary threadProperties)
```
##########
src/log4net/Util/OptionConverter.cs:
##########
@@ -246,8 +227,8 @@ public static bool CanConvertTypeTo(Type sourceType, Type
targetType)
}
// Look for a From converter
- IConvertFrom tcTarget = ConverterRegistry.GetConvertFrom(targetType);
- if (tcTarget != null)
+ IConvertFrom? tcTarget = ConverterRegistry.GetConvertFrom(targetType);
+ if (tcTarget is not null)
Review Comment:
same as above
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -216,11 +188,7 @@ public Hierarchy(PropertiesDictionary properties,
ILoggerFactory loggerFactory)
/// about not having an appender warning.
/// </para>
/// </remarks>
- public bool EmittedNoAppenderWarning
- {
- get { return m_emittedNoAppenderWarning; }
- set { m_emittedNoAppenderWarning = value; }
- }
+ public bool EmittedNoAppenderWarning { get; set; } = false;
Review Comment:
```suggestion
public bool EmittedNoAppenderWarning { get; set; }
```
##########
src/log4net/Util/OptionConverter.cs:
##########
@@ -196,8 +177,8 @@ public static object ConvertStringTo(Type target, string
txt)
// to an arbitrary type T there will be a static method defined on
type T called Parse
// that will take an argument of type string. i.e.
T.Parse(string)->T we call this
// method to convert the string to the type required by the property.
- System.Reflection.MethodInfo meth = target.GetMethod("Parse", new
Type[] { typeof(string) });
- if (meth != null)
+ MethodInfo? meth = target.GetMethod("Parse", new[] { typeof(string)
});
+ if (meth is not null)
Review Comment:
```suggestion
if (target.GetMethod("Parse", new[] { typeof(string) }) is
MethodInfo meth)
```
##########
src/log4net/Repository/Hierarchy/Hierarchy.cs:
##########
@@ -436,12 +394,11 @@ public override void ResetConfiguration()
Root.Level = LevelMap.LookupWithDefault(Level.Debug);
Threshold = LevelMap.LookupWithDefault(Level.All);
- // the synchronization is needed to prevent hashtable surprises
- lock (m_ht)
- {
- Shutdown(); // nested locks are OK
+ Shutdown(); // nested locks are OK
- foreach (Logger l in this.GetCurrentLoggers())
+ foreach (ILogger logger in GetCurrentLoggers())
Review Comment:
same as above
##########
src/log4net/Util/OptionConverter.cs:
##########
@@ -236,8 +217,8 @@ public static bool CanConvertTypeTo(Type sourceType, Type
targetType)
}
// Look for a To converter
- IConvertTo tcSource = ConverterRegistry.GetConvertTo(sourceType,
targetType);
- if (tcSource != null)
+ IConvertTo? tcSource = ConverterRegistry.GetConvertTo(sourceType,
targetType);
+ if (tcSource is not null)
Review Comment:
same as above
##########
src/log4net/Util/ThreadContextStack.cs:
##########
@@ -223,7 +208,7 @@ internal Stack InternalStack
/// Gets the current context information for this stack.
/// </para>
/// </remarks>
- public override string ToString()
+ public override string? ToString()
Review Comment:
```suggestion
public override string? ToString() => GetFullMessage();
```
##########
src/log4net/Util/ReadOnlyPropertiesDictionary.cs:
##########
@@ -178,12 +192,7 @@ public bool Contains(string key)
/// The hashtable used to store the properties
/// </para>
/// </remarks>
- protected Hashtable InnerHashtable
- {
- get { return m_hashtable; }
- }
-
- #region Implementation of ISerializable
+ protected Dictionary<string, object?> InnerHashtable { get; } =
new(StringComparer.Ordinal);
Review Comment:
@fluffynuts I've added this breaking change to the release notes:
https://github.com/apache/logging-log4net/pull/115/commits/db44c9cefd47464407475f28288c97fe0bb1dbe6
##########
src/log4net/Util/OptionConverter.cs:
##########
@@ -290,50 +271,18 @@ public static object ConvertTypeTo(object sourceInstance,
Type targetType)
}
// Look for a FROM converter
- IConvertFrom tcTarget = ConverterRegistry.GetConvertFrom(targetType);
- if (tcTarget != null)
+ IConvertFrom? tcTarget = ConverterRegistry.GetConvertFrom(targetType);
+ if (tcTarget is not null)
Review Comment:
same as above
##########
src/log4net/Util/TypeConverters/ConverterRegistry.cs:
##########
@@ -208,24 +180,19 @@ public static IConvertFrom GetConvertFrom(Type
destinationType)
/// The type converter instance to use for type conversions or <c>null</c>
/// if no type converter is found.
/// </returns>
- private static object GetConverterFromAttribute(Type destinationType)
+ private static object? GetConverterFromAttribute(Type destinationType)
{
// Look for an attribute on the destination type
- var attributes = destinationType
- .GetCustomAttributes(typeof(TypeConverterAttribute), true);
- if (attributes is null)
- {
- // I assume the original null check is perhaps for CF or older .NET
versions -- please leave in place
- return null;
- }
-
+ object[] attributes =
destinationType.GetCustomAttributes(typeof(TypeConverterAttribute), true);
foreach (var attribute in attributes)
{
- var tcAttr = attribute as TypeConverterAttribute;
- if (tcAttr != null)
+ if (attribute is TypeConverterAttribute tcAttr)
{
- var converterType = SystemInfo.GetTypeFromString(destinationType,
tcAttr.ConverterTypeName, false, true);
- return CreateConverterInstance(converterType);
+ Type? converterType = SystemInfo.GetTypeFromString(destinationType,
tcAttr.ConverterTypeName, false, true);
+ if (converterType is not null)
Review Comment:
```suggestion
if (SystemInfo.GetTypeFromString(destinationType,
tcAttr.ConverterTypeName, false, true) is Type converterType)
```
##########
src/log4net/Util/ThreadContextProperties.cs:
##########
@@ -135,20 +112,13 @@ public string[] GetKeys()
/// </remarks>
public void Clear()
Review Comment:
```suggestion
public void Clear() => _dictionary?.Clear();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
