You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "fluffynuts (via GitHub)" <gi...@apache.org> on 2024/11/01 06:03:48 UTC
Re: [PR] use raw strings for config files in unit tests (logging-log4net)
fluffynuts commented on code in PR #203:
URL: https://github.com/apache/logging-log4net/pull/203#discussion_r1825457681
##########
src/log4net.Tests/Appender/RollingFileAppenderTest.cs:
##########
@@ -524,23 +523,10 @@ private static string GetCurrentFile() =>
/// <param name="sBackupGroup"></param>
/// <param name="iBackupFileLength"></param>
/// <returns></returns>
- private static List<RollFileEntry> MakeBackupFileEntriesFromBackupGroup(string sBackupGroup, int iBackupFileLength)
- {
- string[] sFiles = sBackupGroup.Split(' ');
-
- List<RollFileEntry> result = [];
-
- for (int i = 0; i < sFiles.Length; i++)
- {
- // Weed out any whitespace entries from the array
- if (sFiles[i].Trim().Length > 0)
- {
- result.Add(new RollFileEntry(sFiles[i], iBackupFileLength));
- }
- }
-
- return result;
- }
+ private static List<RollFileEntry> MakeBackupFileEntriesFromBackupGroup(string sBackupGroup, int iBackupFileLength)
+ => sBackupGroup.Split(' ').Where(file => file.Trim().Length > 0)
Review Comment:
personal note: i really don't like inline method bodies - it's like I take a little longer to grok them over, eg
```csharp
private static List<RollFileEntry> MakeBackupFileEntriesFromBackupGroup(
string sBackupGroup,
int iBackupFileLength
)
{
return sBackupGroup.Split(' ')
.Where(file => file.Trim().Length > 0)
.Select(file => new RollFileEntry(file, iBackupFileLength))
.ToList()
}
```
Perhaps it's just me though...
I'd also like to make two "soft" suggestions for code style going forward:
1. vertical parameters (as in my example above) - see https://youtu.be/1WL-SudgJAo (I had to re-upload this because it had been removed, so I got it from archive.org and re-upped)
2. dropping type prefixes for variables / parameters (eg _i_BackupFileLength and _s_BackupGroup)
I wouldn't go on a major refactoring spree, just fix things as I'm in the area.
##########
src/log4net.Tests/Appender/AdoNetAppenderTest.cs:
##########
@@ -80,69 +80,70 @@ public void BufferingTest()
}
log.Debug("Message");
Assert.That(Log4NetCommand.MostRecentInstance, Is.Not.Null);
- Assert.That(Log4NetCommand.MostRecentInstance!.ExecuteNonQueryCount, Is.EqualTo(bufferSize + 1));
+ Assert.That(Log4NetCommand.MostRecentInstance.ExecuteNonQueryCount, Is.EqualTo(bufferSize + 1));
}
[Test]
public void WebsiteExample()
{
XmlDocument log4NetConfig = new();
- log4NetConfig.LoadXml(@"
- <log4net>
- <appender name=""AdoNetAppender"" type=""log4net.Appender.AdoNetAppender"">
- <bufferSize value=""-1"" />
- <connectionType value=""log4net.Tests.Appender.AdoNet.Log4NetConnection"" />
- <connectionString value=""data source=[database server];initial catalog=[database name];integrated security=false;persist security info=True;User ID=[user];Password=[password]"" />
- <commandText value=""INSERT INTO Log ([Date],[Thread],[Level],[Logger],[Message],[Exception]) VALUES (@log_date, @thread, @log_level, @logger, @message, @exception)"" />
- <parameter>
- <parameterName value=""@log_date"" />
- <dbType value=""DateTime"" />
- <layout type=""log4net.Layout.RawTimeStampLayout"" />
- </parameter>
- <parameter>
- <parameterName value=""@thread"" />
- <dbType value=""String"" />
- <size value=""255"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%thread"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@log_level"" />
- <dbType value=""String"" />
- <size value=""50"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%level"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@logger"" />
- <dbType value=""String"" />
- <size value=""255"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%logger"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@message"" />
- <dbType value=""String"" />
- <size value=""4000"" />
- <layout type=""log4net.Layout.PatternLayout"">
- <conversionPattern value=""%message"" />
- </layout>
- </parameter>
- <parameter>
- <parameterName value=""@exception"" />
- <dbType value=""String"" />
- <size value=""2000"" />
- <layout type=""log4net.Layout.ExceptionLayout"" />
- </parameter>
- </appender>
- <root>
- <level value=""ALL"" />
- <appender-ref ref=""AdoNetAppender"" />
- </root>
- </log4net>");
+ log4NetConfig.LoadXml("""
Review Comment:
suggest: with raw strings, I find it useful to do them in the format:
```csharp
log4NetConfig.LoadXml(
"""
<log4net>
....
</log4net>
"""
```
so that the stripped whitespace is more obvious (lining up the opening """ with the start/end text and the closing """) - indeed, I had to look up what happens here because Rider always marks raw strings with alignment like this as an issue - to find that the whitespace preceding the _final line_ is stripped from the beginning of each line ( https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/raw-string ). So in the test code here, the entire file looks like it would be prefixed with 2 spaces, where the example I've given is completely left-aligned.
--
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-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org