Merge pull request #834 from SMI/opts

Default values fixes in opts/cli
SMI · Jul 7, 2021 · 556b07e · 556b07e
2 parents 84eee56 + e6a3e01
commit 556b07e
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 45 deletions.
diff --git a/news/834-bugfix.md b/news/834-bugfix.md
@@ -0,0 +1 @@
+Improved logging and fixed yaml options not being respected in IsIdentifiableReviewer
diff --git a/src/applications/Applications.IsIdentifiableReviewer/IsIdentifiableReviewerOptions.cs b/src/applications/Applications.IsIdentifiableReviewer/IsIdentifiableReviewerOptions.cs
@@ -10,6 +10,8 @@ namespace IsIdentifiableReviewer
     /// </summary>
     public class IsIdentifiableReviewerOptions : CliOptions
     {
+        private const string DefaultTargets = "Targets.yaml";
+
         [Option('f', "file",
             Required = false,
             HelpText = "[Optional] Pre load an existing failures file"
@@ -24,24 +26,24 @@ public class IsIdentifiableReviewerOptions : CliOptions
 
         [Option('t', "targets",
             Required = false,
-            Default = "Targets.yaml",
+            Default = DefaultTargets,
             HelpText = "Location of database connection strings file (for issuing UPDATE statements)"
         )]
-        public string TargetsFile { get; set; }
+        public string TargetsFile { get; set; } = DefaultTargets;
 
         [Option('i', "ignore",
             Required = false,
             Default = IgnoreRuleGenerator.DefaultFileName,
             HelpText = "File containing rules for ignoring validation errors"
         )]
-        public string IgnoreList { get; set; }
-        
+        public string IgnoreList { get; set; } = IgnoreRuleGenerator.DefaultFileName;
+
         [Option('r', "redlist",
             Required = false,
             Default = RowUpdater.DefaultFileName,
             HelpText = "File containing rules for when to issue UPDATE statements"
         )]
-        public string RedList { get; set; }
+        public string RedList { get; set; } = RowUpdater.DefaultFileName;
 
 
         [Option('o', "only-rules",
@@ -66,12 +68,20 @@ public class IsIdentifiableReviewerOptions : CliOptions
 
         public virtual void FillMissingWithValuesUsing(IsIdentifiableReviewerGlobalOptions globalOpts)
         {
-            if (string.IsNullOrWhiteSpace(TargetsFile))
-                TargetsFile = globalOpts.TargetsFile;
-            if (string.IsNullOrWhiteSpace(IgnoreList))
-                IgnoreList = globalOpts.IgnoreList;
-            if (string.IsNullOrWhiteSpace(RedList))
-                RedList = globalOpts.RedList;
+            // if we don't have a value for it yet
+            if (string.IsNullOrWhiteSpace(TargetsFile) || TargetsFile == DefaultTargets)
+                // and global configs has got a value for it
+                if(!string.IsNullOrWhiteSpace(globalOpts.TargetsFile))
+                    TargetsFile = globalOpts.TargetsFile; // use the globals config value
+
+            if (string.IsNullOrWhiteSpace(IgnoreList) || IgnoreList == IgnoreRuleGenerator.DefaultFileName)
+                if (!string.IsNullOrWhiteSpace(globalOpts.IgnoreList))
+                    IgnoreList = globalOpts.IgnoreList;
+
+            if (string.IsNullOrWhiteSpace(RedList) || RedList == RowUpdater.DefaultFileName)
+                if (!string.IsNullOrWhiteSpace(globalOpts.RedList))
+                    RedList = globalOpts.RedList;
+
             if (Theme == null && !string.IsNullOrWhiteSpace(globalOpts.Theme))
                 Theme = new FileInfo(globalOpts.Theme);
         }

diff --git a/src/applications/Applications.IsIdentifiableReviewer/Program.cs b/src/applications/Applications.IsIdentifiableReviewer/Program.cs
@@ -1,5 +1,6 @@
 using IsIdentifiableReviewer;
 using IsIdentifiableReviewer.Out;
+using NLog;
 using Smi.Common;
 using Smi.Common.Options;
 using System;
@@ -28,6 +29,7 @@ public static int Main(IEnumerable<string> args)
         private static int OnParse(GlobalOptions globals, IsIdentifiableReviewerOptions opts)
         {
             FansiImplementations.Load();
+            var logger = LogManager.GetCurrentClassLogger();
 
             opts.FillMissingWithValuesUsing(globals.IsIdentifiableReviewerOptions);
 
@@ -40,38 +42,37 @@ private static int OnParse(GlobalOptions globals, IsIdentifiableReviewerOptions
 
                 if (!file.Exists)
                 {
-                    Console.Write($"Could not find '{file.FullName}'");
+                    logger.Error($"Could not find '{file.FullName}'");
                     return 1;
                 }
 
                 var contents = File.ReadAllText(file.FullName);
 
                 if (string.IsNullOrWhiteSpace(contents))
                 {
-                    Console.Write($"Targets file is empty '{file.FullName}'");
+                    logger.Error($"Targets file is empty '{file.FullName}'");
                     return 2;
                 }
 
                 targets = d.Deserialize<List<Target>>(contents);
 
                 if (!targets.Any())
                 {
-                    Console.Write($"Targets file did not contain any valid targets '{file.FullName}'");
+                    logger.Error($"Targets file did not contain any valid targets '{file.FullName}'");
                     return 3;
                 }
             }
             catch (Exception e)
             {
-                Console.WriteLine($"Error deserializing '{opts.TargetsFile}'");
-                Console.WriteLine(e.Message);
+                logger.Error(e,$"Error deserializing '{opts.TargetsFile}'");
                 return 4;
             }
 
             if (opts.OnlyRules)
-                Console.WriteLine("Skipping Connection Tests");
+                logger.Info("Skipping Connection Tests");
             else
             {
-                Console.WriteLine("Running Connection Tests");
+                logger.Info("Running Connection Tests");
 
                 try
                 {
@@ -82,8 +83,7 @@ private static int OnParse(GlobalOptions globals, IsIdentifiableReviewerOptions
                 }
                 catch (Exception e)
                 {
-                    Console.WriteLine("Error Validating Targets");
-                    Console.WriteLine(e.ToString());
+                    logger.Error(e,"Error Validating Targets");
                     return 10;
                 }
             }
@@ -172,7 +172,7 @@ private static int OnParse(GlobalOptions globals, IsIdentifiableReviewerOptions
             }
             catch (Exception e)
             {
-                Console.Write(e.Message);
+                logger.Error(e, $"Application crashed");
 
                 int tries = 5;
                 while (Application.Top != null && tries-- > 0)

diff --git a/src/common/Smi.Common/Options/GlobalOptions.cs b/src/common/Smi.Common/Options/GlobalOptions.cs
@@ -45,28 +45,28 @@ public string HostProcessName
             }
         }
 
-        public LoggingOptions LoggingOptions { get; set; }
-        public RabbitOptions RabbitOptions { get; set; }
-        public FileSystemOptions FileSystemOptions { get; set; }
-        public RDMPOptions RDMPOptions { get; set; }
-        public MongoDatabases MongoDatabases { get; set; }
-        public DicomRelationalMapperOptions DicomRelationalMapperOptions { get; set; }
-        public UpdateValuesOptions UpdateValuesOptions { get; set; }
-        public CohortExtractorOptions CohortExtractorOptions { get; set; }
-        public CohortPackagerOptions CohortPackagerOptions { get; set; }
-        public DicomReprocessorOptions DicomReprocessorOptions { get; set; }
-        public DicomTagReaderOptions DicomTagReaderOptions { get; set; }
-        public FileCopierOptions FileCopierOptions { get; set; }
-        public IdentifierMapperOptions IdentifierMapperOptions { get; set; }
-        public MongoDbPopulatorOptions MongoDbPopulatorOptions { get; set; }
-        public ProcessDirectoryOptions ProcessDirectoryOptions { get; set; }
-        public DeadLetterReprocessorOptions DeadLetterReprocessorOptions { get; set; }
-
-        public TriggerUpdatesOptions TriggerUpdatesOptions {get;set;}
-
-        public IsIdentifiableOptions IsIdentifiableOptions { get; set; }
-        public IsIdentifiableReviewerGlobalOptions IsIdentifiableReviewerOptions { get; set; }
-        public ExtractImagesOptions ExtractImagesOptions { get; set; }
+        public LoggingOptions LoggingOptions { get; set; } = new LoggingOptions();
+        public RabbitOptions RabbitOptions { get; set; } = new RabbitOptions();
+        public FileSystemOptions FileSystemOptions { get; set; } = new FileSystemOptions();
+        public RDMPOptions RDMPOptions { get; set; } = new RDMPOptions();
+        public MongoDatabases MongoDatabases { get; set; } = new MongoDatabases();
+        public DicomRelationalMapperOptions DicomRelationalMapperOptions { get; set; } = new DicomRelationalMapperOptions();
+        public UpdateValuesOptions UpdateValuesOptions { get; set; } = new UpdateValuesOptions();
+        public CohortExtractorOptions CohortExtractorOptions { get; set; } = new CohortExtractorOptions();
+        public CohortPackagerOptions CohortPackagerOptions { get; set; } = new CohortPackagerOptions();
+        public DicomReprocessorOptions DicomReprocessorOptions { get; set; } = new DicomReprocessorOptions();
+        public DicomTagReaderOptions DicomTagReaderOptions { get; set; } = new DicomTagReaderOptions();
+        public FileCopierOptions FileCopierOptions { get; set; } = new FileCopierOptions();
+        public IdentifierMapperOptions IdentifierMapperOptions { get; set; } = new IdentifierMapperOptions();
+        public MongoDbPopulatorOptions MongoDbPopulatorOptions { get; set; } = new MongoDbPopulatorOptions();
+        public ProcessDirectoryOptions ProcessDirectoryOptions { get; set; } = new ProcessDirectoryOptions();
+        public DeadLetterReprocessorOptions DeadLetterReprocessorOptions { get; set; } = new DeadLetterReprocessorOptions();
+
+        public TriggerUpdatesOptions TriggerUpdatesOptions { get; set; } = new TriggerUpdatesOptions();
+
+        public IsIdentifiableOptions IsIdentifiableOptions { get; set; } = new IsIdentifiableOptions();
+        public IsIdentifiableReviewerGlobalOptions IsIdentifiableReviewerOptions { get; set; } = new IsIdentifiableReviewerGlobalOptions();
+        public ExtractImagesOptions ExtractImagesOptions { get; set; } = new ExtractImagesOptions();
 
         #endregion
 

diff --git a/tests/applications/Applications.TriggerUpdates.Tests/MapperSourceUnitTests.cs b/tests/applications/Applications.TriggerUpdates.Tests/MapperSourceUnitTests.cs
@@ -12,8 +12,8 @@ class MapperSourceUnitTests
         [Test]
         public void TestNoIdentifierMapperOptions()
         {
-            var ex = Assert.Throws<Exception>(()=>new MapperSource(new GlobalOptions(),new TriggerUpdatesFromMapperOptions()));
-            Assert.AreEqual("Could not create IdentifierMapper Swapper with SwapperType:Null",ex.Message);
+            var ex = Assert.Throws<ArgumentException>(()=>new MapperSource(new GlobalOptions(),new TriggerUpdatesFromMapperOptions()));
+            Assert.AreEqual("No SwapperType has been specified in GlobalOptions.IdentifierMapperOptions", ex.Message);
 
         }
         [Test]
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Improved logging and fixed yaml options not being respected in IsIdentifiableReviewer