lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on a change in pull request #287: Implemenation of "System Properties" in .NET (addresses #254)
Date Tue, 19 May 2020 04:52:40 GMT

NightOwl888 commented on a change in pull request #287:
URL: https://github.com/apache/lucenenet/pull/287#discussion_r426494910



##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/TestConfigurationRootFactory.cs
##########
@@ -0,0 +1,140 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+//    public class TestConfigurationFactory : DefaultConfigurationFactory
+//    {
+
+//        [CLSCompliant(false)]
+//        public IConfigurationBuilder builder { get; set; }
+
+//        [CLSCompliant(false)]
+//        public TestConfigurationFactory(string currentPath = "", string defaultJsonConfigurationFilename = "luceneTestSettings.json", string defaultXmlConfigurationFilename = "luceneTestSettings.xml", IConfigurationSource[] configurationSources = null) : base(false)
+//        {
+//            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+//            configurationBuilder.AddEnvironmentVariables();
+//            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+//            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+//            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+//            //foreach (IConfigurationSource source in configurationSources)
+//            //{
+//            //    configurationBuilder.Add(source);
+//            //}
+
+//            this.builder = configurationBuilder;
+//        }
+
+//        /// <summary>
+//        /// Initializes the dependencies of this factory.
+//        /// </summary>
+//        [CLSCompliant(false)]
+//        protected override IConfiguration Initialize()
+//        {
+//            return builder.Build();
+//        }
+//    }
+
+//}
+
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class TestConfigurationRootFactory : DefaultConfigurationRootFactory
+    {
+        private readonly ConcurrentDictionary<string, IConfigurationRoot> configurationCache = new ConcurrentDictionary<string, IConfigurationRoot>();
+
+        public string JsonTestSettingsFileName { get; set; } = "lucene.testsettings.json";
+
+        public IConfigurationBuilder builder { get; }
+
+        public TestConfigurationRootFactory() : base(false)
+        {
+
+            //configurationBuilder.AddEnvironmentVariables();
+            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+            //this.builder = configurationBuilder;
+        }
+
+        public override IConfigurationRoot CreateConfiguration()
+        {
+            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+            string testDirectory =
+#if TESTFRAMEWORK_NUNIT
+                NUnit.Framework.TestContext.CurrentContext.TestDirectory;
+#elif NETSTANDARD

Review comment:
       `AppDomain` is available in .NET Standard (except for 1.x). Please remove this elif.

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/ConfigurationBuilderExtensionMethods.cs
##########
@@ -0,0 +1,83 @@
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Configuration.Json;
+using Microsoft.Extensions.Configuration.Xml;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    public static class ConfigurationBuilderExtensions

Review comment:
       Please rename the file to match the class name

##########
File path: src/Lucene.Net.Analysis.SmartCn/AnalyzerProfile.cs
##########
@@ -69,7 +69,8 @@ private static void Init()
 
             // Try the system property:-Danalysis.data.dir=/path/to/analysis-data
             //ANALYSIS_DATA_DIR = System.getProperty("analysis.data.dir", "");
-            ANALYSIS_DATA_DIR = SystemProperties.GetProperty("analysis.data.dir", "");
+            // LUCENENET specific - reformatted with :
+            ANALYSIS_DATA_DIR = SystemProperties.GetProperty("analysis:data:dir", "");

Review comment:
       Please change this property name to `smartcn:data:dir`, including the documentation comments that reference it

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,83 @@
+using Lucene.Net.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public abstract class ConfigurationSettings
+    {
+        private static IConfigurationRootFactory configurationFactory = new DefaultConfigurationRootFactory(false);
+
+        /// <summary>
+        /// Sets the <see cref="IConfigurationRootFactory"/> instance used to instantiate
+        /// <see cref="ConfigurationSettings"/> subclasses.
+        /// </summary>
+        /// <param name="configurationFactory">The new <see cref="IConfigurationRootFactory"/>.</param>
+        /// <exception cref="ArgumentNullException">The <paramref name="configurationFactory"/> parameter is <c>null</c>.</exception>
+        [CLSCompliant(false)]
+        public static void SetConfigurationFactory(IConfigurationRootFactory configurationFactory)

Review comment:
       Please change the name of this method to `SetConfigurationRootFactory` and change the name of the parameter to `configurationRootFactory`.

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/TestConfigurationRootFactory.cs
##########
@@ -0,0 +1,140 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+//    public class TestConfigurationFactory : DefaultConfigurationFactory
+//    {
+
+//        [CLSCompliant(false)]
+//        public IConfigurationBuilder builder { get; set; }
+
+//        [CLSCompliant(false)]
+//        public TestConfigurationFactory(string currentPath = "", string defaultJsonConfigurationFilename = "luceneTestSettings.json", string defaultXmlConfigurationFilename = "luceneTestSettings.xml", IConfigurationSource[] configurationSources = null) : base(false)
+//        {
+//            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+//            configurationBuilder.AddEnvironmentVariables();
+//            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+//            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+//            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+//            //foreach (IConfigurationSource source in configurationSources)
+//            //{
+//            //    configurationBuilder.Add(source);
+//            //}
+
+//            this.builder = configurationBuilder;
+//        }
+
+//        /// <summary>
+//        /// Initializes the dependencies of this factory.
+//        /// </summary>
+//        [CLSCompliant(false)]
+//        protected override IConfiguration Initialize()
+//        {
+//            return builder.Build();
+//        }
+//    }
+
+//}
+
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class TestConfigurationRootFactory : DefaultConfigurationRootFactory
+    {
+        private readonly ConcurrentDictionary<string, IConfigurationRoot> configurationCache = new ConcurrentDictionary<string, IConfigurationRoot>();
+
+        public string JsonTestSettingsFileName { get; set; } = "lucene.testsettings.json";
+
+        public IConfigurationBuilder builder { get; }
+
+        public TestConfigurationRootFactory() : base(false)
+        {
+
+            //configurationBuilder.AddEnvironmentVariables();
+            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+            //this.builder = configurationBuilder;
+        }
+
+        public override IConfigurationRoot CreateConfiguration()
+        {
+            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+            string testDirectory =
+#if TESTFRAMEWORK_NUNIT
+                NUnit.Framework.TestContext.CurrentContext.TestDirectory;
+#elif NETSTANDARD
+                System.AppContext.BaseDirectory;
+#else
+                AppDomain.CurrentDomain.BaseDirectory;
+#endif
+
+            return configurationCache.GetOrAdd(testDirectory, (key) =>
+            {
+                return new ConfigurationBuilder()
+                    .AddLuceneDefaultSettings(prefix: "lucene:") // Use a custom prefix to only load Lucene.NET settings
+                    .AddJsonFilesFromRootDirectoryTo(currentPath: key, JsonTestSettingsFileName)
+#if TESTFRAMEWORK_NUNIT
+                    .AddNUnitTestRunSettings()
+#endif
+                    .Build();
+            });
+        }
+            ///// <summary>

Review comment:
       Please remove this comment block

##########
File path: src/Lucene.Net/Support/Util/SystemProperties.cs
##########
@@ -163,7 +149,8 @@ private static T GetProperty<T>(string key, T defaultValue, Func<string, T> conv
         /// <exception cref="SecurityException">The caller does not have the required permission to perform this operation.</exception>
         public static void SetProperty(string key, string value)

Review comment:
       As we had previously discussed, please remove `SetProperty` from` SystemProperties`. It will be confusing when porting in the future to have a method with the same name but different behavior as Java.
   
   All tests that call this method should be converted to use the .NET-centric `ConfigurationSettings.CurrentConfiguration` API to set configuration settings, if needed.

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationRootFactory.cs
##########
@@ -0,0 +1,108 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    internal class DefaultConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+        private bool initialized = false;
+        protected object m_initializationLock = new object();
+        private IConfigurationBuilder builder { get; }
+        private IConfigurationRoot configuration;
+
+        public DefaultConfigurationRootFactory(bool ignoreSecurityExceptionsOnRead)
+        {
+            this.builder = new ConfigurationBuilder();
+            builder.Add(new LuceneDefaultConfigurationSource() { Prefix = "lucene:" });
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        public virtual IConfigurationRoot CreateConfiguration()
+        {
+            return EnsureInitialized();
+        }
+
+        /// <summary>
+        /// Ensures the <see cref="Initialize"/> method has been called since the
+        /// last application start. This method is thread-safe.
+        /// </summary>
+        protected IConfigurationRoot EnsureInitialized()
+        {
+            return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized, ref this.m_initializationLock, () =>
+            {
+                return Initialize();
+            });
+        }
+
+        /// <summary>
+        /// Initializes the dependencies of this factory.
+        /// </summary>
+        protected virtual IConfigurationRoot Initialize()
+        {
+            return builder.Build();
+        }
+        /// <summary>
+        /// Reloads the dependencies of this factory.
+        /// </summary>
+        public void ReloadConfiguration()
+        {
+            CreateConfiguration().Reload();
+        }
+    }
+    //internal class DefaultConfiguration : IConfiguration

Review comment:
       Please do not place commented code in a PR commit unless it is something from Java that we decided not to use. Git keeps a commit history of each copy of the code that can be looked up fairly easily. Or, if preferred, a separate branch can be made to keep a record of a certain state that you can refer back to by switching branches.
   
   The GitHub review process shows the diff between the state of the target branch and the state of the PR branch across all commits. This is meant to minimize the amount of text to be reviewed (if something is added in one commit, and later removed in another commit it doesn't show up in the review). However, commented code is always shown even if it is not relevant to the review.
   
   A good rule of thumb is to always aim to keep the PR branch in a state where it is ready to be merged. Anything you think is worth keeping around for reference, do so in another branch or make a backup of the file outside of the working directory.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,95 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");
+            Assert.AreEqual("en", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please update to use `ConfigurationSettings.CurrentSettings` to set the property.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,95 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);

Review comment:
       Please update to use `ConfigurationSettings.CurrentSettings` to set the property.

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/TestConfigurationRootFactory.cs
##########
@@ -0,0 +1,140 @@
+//using Lucene.Net.Configuration;

Review comment:
       Please remove this comment block

##########
File path: src/Lucene.Net/Support/Configuration/Base/ConfigurationSection.cs
##########
@@ -0,0 +1,118 @@
+// Copyright (c) .NET Foundation. All rights reserved.

Review comment:
       Please ensure this license is at the top of every file copied from Microsoft sources. Also, add this and all of the files with non-default license headers in [`.rat-excludes`](https://github.com/apache/lucenenet/blob/master/.rat-excludes).
   
   > As part of the release process, we run an audit to ensure that every file has a default header. However, if another license covers the file, it should be excluded from the audit.

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,83 @@
+using Lucene.Net.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public abstract class ConfigurationSettings
+    {
+        private static IConfigurationRootFactory configurationFactory = new DefaultConfigurationRootFactory(false);

Review comment:
       Please change the variable name to `configurationRootFactory`

##########
File path: src/Lucene.Net.TestFramework/Lucene.Net.TestFramework.csproj
##########
@@ -58,20 +58,54 @@
     <PackageReference Include="NUnit" Version="$(NUnitPackageVersion)" />
   </ItemGroup>
 
-  <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.6' ">
+  <!--<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.6' ">
     <PackageReference Include="System.Diagnostics.StackTrace" Version="$(SystemDiagnosticsStackTracePackageVersion)" />
     <PackageReference Include="System.Diagnostics.TraceSource" Version="$(SystemDiagnosticsTraceSourcePackageVersion)" />
     <PackageReference Include="System.Net.Primitives" Version="$(SystemNetPrimitivesPackageVersion)" />
     <PackageReference Include="System.Threading.Thread" Version="$(SystemThreadingThreadPackageVersion)" />
+  </ItemGroup>-->
+
+
+  <ItemGroup>
   </ItemGroup>
 
-  <ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="$(MicrosoftExtensionsConfigurationPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="$(MicrosoftExtensionsConfigurationJsonPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Xml" Version="$(MicrosoftExtensionsConfigurationXmlPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="$(MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="$(MicrosoftExtensionsConfigurationCommandLinePackageVersion)" />
+  </ItemGroup>
+
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.1'">
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="$(MicrosoftExtensionsConfigurationJsonPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Xml" Version="$(MicrosoftExtensionsConfigurationXmlPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="$(MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="$(MicrosoftExtensionsConfigurationCommandLinePackageVersion)" />
+  </ItemGroup>
+  
+  <ItemGroup Condition=" '$(TargetFramework)' == 'net451' ">
     <Reference Include="System.IO.Compression" />
     <Reference Include="System.Numerics" />
     <Reference Include="System.ServiceModel" />
     <Reference Include="System" />
     <Reference Include="Microsoft.CSharp" />
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="$(MicrosoftExtensionsConfigurationPackageVersion_NET4_5_1)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="$(MicrosoftExtensionsConfigurationJsonPackageVersion_NET4_5_1)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Xml" Version="$(MicrosoftExtensionsConfigurationXmlPackageVersion_NET4_5_1)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="$(MicrosoftExtensionsConfigurationEnvironmentVariablesPackageVersion_NET4_5_1)" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.CommandLine" Version="$(MicrosoftExtensionsConfigurationCommandLinePackageVersion_NET4_5_1)" />
   </ItemGroup>
+  
+  <!--<ItemGroup Condition=" '$(TargetFramework)' == 'net45' ">

Review comment:
       Please remove this ItemGroup element

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,83 @@
+using Lucene.Net.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public abstract class ConfigurationSettings

Review comment:
       Please change this to `public static` so this class cannot be inherited

##########
File path: src/dotnet/tools/lucene-cli/Configuration/ConfigurationRootFactory.cs
##########
@@ -0,0 +1,82 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class ConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly ConcurrentDictionary<string, IConfigurationRoot> configurationCache = new ConcurrentDictionary<string, IConfigurationRoot>();
+
+        public string JsonTestSettingsFileName { get; set; } = "appsettings.json";
+
+        public IConfigurationBuilder builder { get; }
+
+        public IConfigurationRoot CreateConfiguration()
+        {
+            string testDirectory =
+#if NETSTANDARD

Review comment:
       Please remove this conditional compilation. .NET Standard 1.x is the only framework that doesn't support `AppDomain.CurrentDomain.BaseDirectory`, and this application is only supported on .NET Core 3.1

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationRootFactory.cs
##########
@@ -0,0 +1,108 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    internal class DefaultConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+        private bool initialized = false;
+        protected object m_initializationLock = new object();
+        private IConfigurationBuilder builder { get; }
+        private IConfigurationRoot configuration;
+
+        public DefaultConfigurationRootFactory(bool ignoreSecurityExceptionsOnRead)
+        {
+            this.builder = new ConfigurationBuilder();
+            builder.Add(new LuceneDefaultConfigurationSource() { Prefix = "lucene:" });
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        public virtual IConfigurationRoot CreateConfiguration()
+        {
+            return EnsureInitialized();
+        }
+
+        /// <summary>
+        /// Ensures the <see cref="Initialize"/> method has been called since the
+        /// last application start. This method is thread-safe.
+        /// </summary>
+        protected IConfigurationRoot EnsureInitialized()
+        {
+            return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized, ref this.m_initializationLock, () =>
+            {
+                return Initialize();
+            });
+        }
+
+        /// <summary>
+        /// Initializes the dependencies of this factory.
+        /// </summary>
+        protected virtual IConfigurationRoot Initialize()
+        {
+            return builder.Build();
+        }
+        /// <summary>
+        /// Reloads the dependencies of this factory.
+        /// </summary>
+        public void ReloadConfiguration()

Review comment:
       Please remove this method both here and from the underlying interface.

##########
File path: src/Lucene.Net/Support/Util/SystemProperties.cs
##########
@@ -131,30 +133,14 @@ public static int GetPropertyAsInt32(string key, int defaultValue)
 

Review comment:
       Please mark `SystemProperties` internal. .NET users should only use the more familiar `IConfigurtion` (root) interface to interact with configuration, including its behaviors.
   
   Also, please update the documentation comments here, as none of this applies specifically to environment variables anymore.

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,83 @@
+using Lucene.Net.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public abstract class ConfigurationSettings
+    {
+        private static IConfigurationRootFactory configurationFactory = new DefaultConfigurationRootFactory(false);
+
+        /// <summary>
+        /// Sets the <see cref="IConfigurationRootFactory"/> instance used to instantiate
+        /// <see cref="ConfigurationSettings"/> subclasses.
+        /// </summary>
+        /// <param name="configurationFactory">The new <see cref="IConfigurationRootFactory"/>.</param>
+        /// <exception cref="ArgumentNullException">The <paramref name="configurationFactory"/> parameter is <c>null</c>.</exception>
+        [CLSCompliant(false)]
+        public static void SetConfigurationFactory(IConfigurationRootFactory configurationFactory)
+        {
+            ConfigurationSettings.configurationFactory = configurationFactory ?? throw new ArgumentNullException(nameof(configurationFactory));
+        }
+
+        /// <summary>
+        /// Gets the associated ConfigurationSettings factory.
+        /// </summary>
+        /// <returns>The ConfigurationSettings factory.</returns>
+        [CLSCompliant(false)]
+        public static IConfigurationRootFactory GetConfigurationFactory()
+        {
+            return configurationFactory;
+        }
+
+        public static void Reload()

Review comment:
       Please remove `Reload()`. Instead, add a property to get the `ConfigurationRoot`
   
   ```c#
   [CLSCompliant(false)]
   public static IConfigurationRoot CurrentConfiguration =>
       configurationFactory.CreateConfiguration();
   ```

##########
File path: src/Lucene.Net/Support/Configuration/IConfigurationFactory.cs
##########
@@ -0,0 +1,31 @@
+using Microsoft.Extensions.Configuration;

Review comment:
       Please rename the file to match the type name

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationRootFactory.cs
##########
@@ -0,0 +1,108 @@
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    internal class DefaultConfigurationRootFactory : IConfigurationRootFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+        private bool initialized = false;
+        protected object m_initializationLock = new object();
+        private IConfigurationBuilder builder { get; }

Review comment:
       Please use a readonly field, not a property.

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);

Review comment:
       Please change to
   
   ```c#
   Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
   ```

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");
+            Assert.AreEqual("en", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+        }
+
+        [Test]
+        public virtual void TestSetandUnset()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please change to
   ```c#
   Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
   ```

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,96 @@
+using Lucene.Net.Configuration;
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please change to
   ```c#
   Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration["tests:locale"] = "en";
   ```

##########
File path: src/Lucene.Net/Support/Configuration/IConfigurationFactory.cs
##########
@@ -0,0 +1,31 @@
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+    [CLSCompliant(false)]
+    public interface IConfigurationRootFactory
+    {
+        IConfigurationRoot CreateConfiguration();
+        void ReloadConfiguration();

Review comment:
       Please remove `ReloadConfiguration()`. It is not always sensible to have such a feature for every `IConfigurationRootFactory` implementation.

##########
File path: src/Lucene.Net/Lucene.Net.csproj
##########
@@ -41,6 +41,9 @@
 
   <ItemGroup Label="NuGet Package Files">
     <None Include="$(LuceneNetCodeAnalysisDir)tools\*.ps1" Pack="true" PackagePath="tools" />
+    <Compile Remove="Support\NewFolder\**" />

Review comment:
       What are these for?

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/ConfigurationBuilderExtensionMethods.cs
##########
@@ -0,0 +1,83 @@
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Configuration.Json;
+using Microsoft.Extensions.Configuration.Xml;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    public static class ConfigurationBuilderExtensions
+    {
+
+        [CLSCompliant(false)]
+        public static IConfigurationBuilder AddLuceneDefaultSettings(this IConfigurationBuilder configurationBuilder, string prefix)
+        {
+            configurationBuilder.Add(new LuceneDefaultConfigurationSource() { Prefix = prefix });

Review comment:
       Please return the `IConfigurationBuilder` from the `Add` method.

##########
File path: src/dotnet/tools/Lucene.Net.Tests.Cli/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Cli.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Util.SystemProperties.GetProperty(testKey));
+        }
+        [Test]
+        public virtual void SetEnvironmentTest()
+        {
+            string testKey  = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);

Review comment:
       Please change to
   
   ```c#
   Lucene.Net.Configuration.ConfigurationSettings.CurrentConfiguration[testKey] = testValue;
   ```

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -685,13 +689,17 @@ public virtual void TearDown()
             /* LUCENENET TODO: Not sure how to convert these
                 ParentChainCallRule.TeardownCalled = true;
                 */
+            ConfigurationSettings.Reload();
         }
 
         // LUCENENET specific constants to scan the test framework for codecs/docvaluesformats/postingsformats only once
         public static ICodecFactory CodecFactory { get; set; } = new TestCodecFactory();
         public static IDocValuesFormatFactory DocValuesFormatFactory { get; set; } = new TestDocValuesFormatFactory();
         public static IPostingsFormatFactory PostingsFormatFactory { get; set; } = new TestPostingsFormatFactory();
 
+        [CLSCompliant(false)]
+        public static IConfigurationRootFactory ConfigurationFactory { get; set; } = new TestConfigurationRootFactory();
+        //public static IConfigurationFactory ConfigurationFactory { get; set; } = new DefaultConfigurationFactory(false);

Review comment:
       Please remove this comment

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestDefaultSystemProperties.cs
##########
@@ -0,0 +1,37 @@
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestDefaultSystemProperties : LuceneTestCase
+    {
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    //ConfigurationFactory = new DefaultConfigurationFactory(false);
+        //    //base.BeforeClass();
+        //}
+        [Test]
+        public virtual void ReadEnvironmentTest()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Assert.AreEqual(testValue, Lucene.Net.Util.SystemProperties.GetProperty(testKey));
+        }
+        [Test]
+        public virtual void SetEnvironmentTest()
+        {
+            string testKey  = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);

Review comment:
       Please update to use `ConfigurationSettings.CurrentSettings` to set the property.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -0,0 +1,95 @@
+using Lucene.Net.Search;
+using Lucene.Net.Util;
+using Microsoft.Extensions.Configuration;
+using Microsoft.VisualBasic.CompilerServices;
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+
+
+    [TestFixture]
+    class TestSystemProperties : LuceneTestCase
+    {
+
+        //[OneTimeSetUp]
+        //public override void BeforeClass()
+        //{
+        //    ConfigurationFactory = new TestConfigurationFactory();
+        //    base.BeforeClass();
+        //}
+        [Test]
+        public virtual void EnvironmentTest2()
+        {
+            string testKey = "lucene:tests:setting";
+            string testValue = "test.success";
+            Lucene.Net.Util.SystemProperties.SetProperty(testKey, testValue);
+            Assert.AreEqual(Lucene.Net.Util.SystemProperties.GetProperty(testKey), testValue);
+        }
+        [Test]
+        public virtual void SetTest()
+        {
+            Assert.AreEqual("fr", Lucene.Net.Util.SystemProperties.GetProperty("tests:locale"));
+            Lucene.Net.Util.SystemProperties.SetProperty("tests:locale", "en");

Review comment:
       Please update to use `ConfigurationSettings.CurrentSettings` to set the property.

##########
File path: src/Lucene.Net/Support/Configuration/ConfigurationSettings.cs
##########
@@ -0,0 +1,83 @@
+using Lucene.Net.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    public abstract class ConfigurationSettings
+    {
+        private static IConfigurationRootFactory configurationFactory = new DefaultConfigurationRootFactory(false);
+
+        /// <summary>
+        /// Sets the <see cref="IConfigurationRootFactory"/> instance used to instantiate
+        /// <see cref="ConfigurationSettings"/> subclasses.
+        /// </summary>
+        /// <param name="configurationFactory">The new <see cref="IConfigurationRootFactory"/>.</param>
+        /// <exception cref="ArgumentNullException">The <paramref name="configurationFactory"/> parameter is <c>null</c>.</exception>
+        [CLSCompliant(false)]
+        public static void SetConfigurationFactory(IConfigurationRootFactory configurationFactory)
+        {
+            ConfigurationSettings.configurationFactory = configurationFactory ?? throw new ArgumentNullException(nameof(configurationFactory));
+        }
+
+        /// <summary>
+        /// Gets the associated ConfigurationSettings factory.
+        /// </summary>
+        /// <returns>The ConfigurationSettings factory.</returns>
+        [CLSCompliant(false)]
+        public static IConfigurationRootFactory GetConfigurationFactory()

Review comment:
       Please change the name of this method to `GetConfigurationRootFactory()`

##########
File path: src/dotnet/tools/lucene-cli/ConfigurationBase.cs
##########
@@ -46,6 +48,9 @@ protected ConfigurationBase()
             {
                 return ShortVersionGetter();
             };
+
+            // Setup the factories
+            ConfigurationSettings.SetConfigurationFactory(ConfigurationFactory);

Review comment:
       Please set the factory inside of `Program.Main()` (the composition root) and revert the changes to this class. This ensures it is only set once when the application runs.
   
   ```c#
       public class Program
       {
           public static int Main(string[] args)
           {
               ConfigurationSettings.SetConfigurationFactory(new ConfigurationRootFactory());
   
               int result = CommandLineOptions.Parse(args);
   
   #if DEBUG
               Console.ReadKey();
   #endif
   
               return result;
           }
       }
   ```

##########
File path: src/Lucene.Net.TestFramework/Util/TestRuleSetupAndRestoreClassEnv.cs
##########
@@ -287,7 +287,8 @@ public override void Before(LuceneTestCase testInstance)
 
             // TimeZone.getDefault will set user.timezone to the default timezone of the user's locale.
             // So store the original property value and restore it at end.
-            restoreProperties["user.timezone"] = SystemProperties.GetProperty("user.timezone");
+            // LUCENENET specific - commented
+            //restoreProperties["user:timezone"] = SystemProperties.GetProperty("user:timezone");

Review comment:
       Please comment the below block and the above `restoreProperties` member variable declaration with the message `// LUCENENT specific - Not used in .NET`
   
   ```c#
   foreach (KeyValuePair<string, string> e in restoreProperties)
   {
   	SystemProperties.SetProperty(e.Key, e.Value);
   }
   restoreProperties.Clear();
   ```

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/TestConfigurationRootFactory.cs
##########
@@ -0,0 +1,140 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+//    public class TestConfigurationFactory : DefaultConfigurationFactory
+//    {
+
+//        [CLSCompliant(false)]
+//        public IConfigurationBuilder builder { get; set; }
+
+//        [CLSCompliant(false)]
+//        public TestConfigurationFactory(string currentPath = "", string defaultJsonConfigurationFilename = "luceneTestSettings.json", string defaultXmlConfigurationFilename = "luceneTestSettings.xml", IConfigurationSource[] configurationSources = null) : base(false)
+//        {
+//            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+//            configurationBuilder.AddEnvironmentVariables();
+//            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+//            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+//            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+//            //foreach (IConfigurationSource source in configurationSources)
+//            //{
+//            //    configurationBuilder.Add(source);
+//            //}
+
+//            this.builder = configurationBuilder;
+//        }
+
+//        /// <summary>
+//        /// Initializes the dependencies of this factory.
+//        /// </summary>
+//        [CLSCompliant(false)]
+//        protected override IConfiguration Initialize()
+//        {
+//            return builder.Build();
+//        }
+//    }
+
+//}
+
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class TestConfigurationRootFactory : DefaultConfigurationRootFactory
+    {
+        private readonly ConcurrentDictionary<string, IConfigurationRoot> configurationCache = new ConcurrentDictionary<string, IConfigurationRoot>();
+
+        public string JsonTestSettingsFileName { get; set; } = "lucene.testsettings.json";
+
+        public IConfigurationBuilder builder { get; }

Review comment:
       Please remove this property

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/TestConfigurationRootFactory.cs
##########
@@ -0,0 +1,140 @@
+//using Lucene.Net.Configuration;
+//using Microsoft.Extensions.Configuration;
+//using System;
+//using System.Collections.Generic;
+
+//namespace Lucene.Net.Configuration
+//{
+//    /*
+//     * Licensed to the Apache Software Foundation (ASF) under one or more
+//     * contributor license agreements.  See the NOTICE file distributed with
+//     * this work for additional information regarding copyright ownership.
+//     * The ASF licenses this file to You under the Apache License, Version 2.0
+//     * (the "License"); you may not use this file except in compliance with
+//     * the License.  You may obtain a copy of the License at
+//     *
+//     *     http://www.apache.org/licenses/LICENSE-2.0
+//     *
+//     * Unless required by applicable law or agreed to in writing, software
+//     * distributed under the License is distributed on an "AS IS" BASIS,
+//     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//     * See the License for the specific language governing permissions and
+//     * limitations under the License.
+//     */
+
+//    public class TestConfigurationFactory : DefaultConfigurationFactory
+//    {
+
+//        [CLSCompliant(false)]
+//        public IConfigurationBuilder builder { get; set; }
+
+//        [CLSCompliant(false)]
+//        public TestConfigurationFactory(string currentPath = "", string defaultJsonConfigurationFilename = "luceneTestSettings.json", string defaultXmlConfigurationFilename = "luceneTestSettings.xml", IConfigurationSource[] configurationSources = null) : base(false)
+//        {
+//            IConfigurationBuilder configurationBuilder = new ConfigurationBuilder();
+
+//            configurationBuilder.AddEnvironmentVariables();
+//            //configurationBuilder.AddXmlFilesFromRootDirectoryTo(currentPath, defaultXmlConfigurationFilename);
+//            //configurationBuilder.AddJsonFilesFromRootDirectoryTo(currentPath, defaultJsonConfigurationFilename);
+//            //configurationBuilder.Add(new TestParameterConfigurationSource(NUnit.Framework.TestContext.Parameters));
+
+//            //foreach (IConfigurationSource source in configurationSources)
+//            //{
+//            //    configurationBuilder.Add(source);
+//            //}
+
+//            this.builder = configurationBuilder;
+//        }
+
+//        /// <summary>
+//        /// Initializes the dependencies of this factory.
+//        /// </summary>
+//        [CLSCompliant(false)]
+//        protected override IConfiguration Initialize()
+//        {
+//            return builder.Build();
+//        }
+//    }
+
+//}
+
+
+using Lucene.Net.Configuration;
+using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+
+    internal class TestConfigurationRootFactory : DefaultConfigurationRootFactory
+    {
+        private readonly ConcurrentDictionary<string, IConfigurationRoot> configurationCache = new ConcurrentDictionary<string, IConfigurationRoot>();
+
+        public string JsonTestSettingsFileName { get; set; } = "lucene.testsettings.json";
+
+        public IConfigurationBuilder builder { get; }
+
+        public TestConfigurationRootFactory() : base(false)
+        {
+
+            //configurationBuilder.AddEnvironmentVariables();

Review comment:
       Pleae remove this comment block

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -767,6 +775,8 @@ public virtual void BeforeClass()
         {
             try
             {
+                // Setup the factories
+                ConfigurationSettings.SetConfigurationFactory(ConfigurationFactory);
                 // Setup the factories

Review comment:
       Please remove this duplicate comment

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/ConfigurationBuilderExtensionMethods.cs
##########
@@ -0,0 +1,83 @@
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Configuration.Json;
+using Microsoft.Extensions.Configuration.Xml;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Security;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    public static class ConfigurationBuilderExtensions
+    {
+
+        [CLSCompliant(false)]
+        public static IConfigurationBuilder AddLuceneDefaultSettings(this IConfigurationBuilder configurationBuilder, string prefix)
+        {
+            configurationBuilder.Add(new LuceneDefaultConfigurationSource() { Prefix = prefix });
+            return configurationBuilder;
+        }
+        [CLSCompliant(false)]
+        public static IConfigurationBuilder AddNUnitTestRunSettings(this IConfigurationBuilder configurationBuilder)
+        {
+            configurationBuilder.Add(new TestParameterConfigurationSource() { TestParameters = NUnit.Framework.TestContext.Parameters });

Review comment:
       Please return the `IConfigurationBuilder` from the `Add` method.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message