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 Mon, 25 May 2020 12:10:21 GMT

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



##########
File path: src/Lucene.Net/Support/Util/Properties.cs
##########
@@ -0,0 +1,154 @@
+using Microsoft.Extensions.Configuration;
+using System;
+
+namespace Lucene.Net.Util
+{
+    /*
+     * 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.
+     */
+
+    /// <summary>
+    /// Implementation of <see cref="IProperties"/> that handles type conversion and
default values 
+    /// for Java-style properties.
+    /// <para/>
+    /// Reads properties from an <see cref="IConfigurationRoot" /> instance or
+    /// a <see cref="Func{IConfigurationRoot}"/> that is supplied to the constructor.
+    /// </summary>
+    internal class Properties : IProperties
+    {
+        private readonly Func<IConfigurationRoot> createConfiguration;
+
+        /// <summary>
+        /// Initaializes a new instance of <see cref="Properties"/> with the specified
<see cref="IConfigurationRoot"/>.
+        /// </summary>
+        /// <param name="configuration">The <see cref="IConfigurationRoot"/>.</param>
+        public Properties(IConfigurationRoot configuration)

Review comment:
       Please remove this constructor and only use the delegate method constructor (less to
maintain).

##########
File path: src/Lucene.Net/Support/Configuration/IConfigurationRootFactory.cs
##########
@@ -0,0 +1,28 @@
+using Microsoft.Extensions.Configuration;
+using System;
+
+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 CurrentConfiguration { get; }

Review comment:
       Please change this back to using a method named `CreateConfiguration`, as the [abstract
factory pattern typically uses a method](https://blog.ploeh.dk/2014/05/19/di-friendly-framework/)
and using a property makes it confusing.

##########
File path: src/dotnet/tools/lucene-cli/Configuration/ConfigurationRootFactory.cs
##########
@@ -0,0 +1,70 @@
+//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 Microsoft.Extensions.Configuration;
+using System.Collections.Concurrent;
+
+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 : NamedConfigurationRootFactory, IConfigurationRootFactory
+    {
+        protected IConfigurationRoot configuration;
+
+        public IConfigurationRoot CurrentConfiguration
+        {
+            get
+            {
+                EnsureInitialized();
+                return configuration;
+            }
+        }
+
+        /// <summary>
+        /// Initializes the dependencies of this factory.
+        /// </summary>
+        protected override void Initialize()
+        {
+            IConfigurationBuilder builder = new ConfigurationBuilder()

Review comment:
       In a console application, the builder should be run inside of the composition root
before the factory is created. There is no reason to lazy load here because we know that will
happen exactly once at application start up and there is no way to override it in this application.
   
   ```c#
       internal class ConfigurationRootFactory : IConfigurationRootFactory
       {
           private readonly IConfigurationRoot configuration;
   
           public ConfigurationRootFactory(IConfigurationRoot configuration)
           {
               this.configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
           }
   
           public IConfigurationRoot CreateConfiguration()
           {
               return configuration;
           }
       }
   
       public class Program
       {
           public static int Main(string[] args)
           {
               var configuration = new ConfigurationBuilder()
                   .AddEnvironmentVariables(prefix: "lucene:") // Use a custom prefix to only
load Lucene.NET settings 
                   .AddJsonFile("appsettings.json")
                   .Build();
               ConfigurationSettings.SetConfigurationRootFactory(new ConfigurationRootFactory(configuration));
   
               int result = CommandLineOptions.Parse(args);
   
   #if DEBUG
               Console.ReadKey();
   #endif
   
               return result;
           }
       }
   ```
   

##########
File path: src/Lucene.Net/Support/Configuration/NamedConfigurationRootFactory.cs
##########
@@ -0,0 +1,46 @@
+using Microsoft.Extensions.Configuration;
+using System.Threading;
+
+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 NamedConfigurationRootFactory

Review comment:
       Please eliminate this class and revert the implementation of `DefaultConfigurationRootFactory`
to its original implemenation that uses `LazyInitializer` internally.

##########
File path: src/Lucene.Net/Support/Util/SystemProperties.cs
##########
@@ -21,42 +23,31 @@ namespace Lucene.Net.Util
      */
 
     /// <summary>
-    /// Helper for environment variables. This class helps to convert the environment
-    /// variables to int or bool data types and also silently handles read permission
-    /// errors.
-    /// <para/>
-    /// For instructions how to set environment variables for your OS, see 
-    /// <a href="https://www.schrodinger.com/kb/1842">https://www.schrodinger.com/kb/1842</a>.
-    /// <para/>
-    /// Note that if you want to load any of these settings for your application from a
-    /// configuration file, it is recommended your application load them at startup and
-    /// call <see cref="SystemProperties.SetProperty(string, string)"/> to set them.
-    /// <para/>
-    /// Set the environment variable <c>lucene.ignoreSecurityExceptions</c> to
<c>false</c>
-    /// to change the read behavior of these methods to throw the underlying exception 
-    /// instead of returning the default value.
+    /// Reads properties from an IConfiguration(Root) object returned by a ConfigurationFactory.
+    /// The ConfigurationFactory is set using ConfigurationSettings.SetConfigurationRootFactory().
+    /// This can be supplied a user implemented factory (IConfigurationRootFactory) 
     /// </summary>
-    public static class SystemProperties
+    internal static class SystemProperties

Review comment:
       Please use the following implementation:
   
   ```c#
       /// <summary>
       /// Reads properties from an <see cref="IProperties"/> instance. The default
configuration reads
       /// the property valies from an <see cref="IConfigurationRoot"/> instance returned
by a
       /// <see cref="IConfigurationRootFactory"/> implementation.
       /// The <see cref="IConfigurationRootFactory"/> is set using
       /// <see cref="ConfigurationSettings.SetConfigurationRootFactory(IConfigurationRootFactory)"/>.
       /// This can be supplied a user implemented <see cref="IConfigurationRootFactory"/>
to customize
       /// the property sources.
       /// </summary>
       internal static class SystemProperties
       {
           // Calls ConfigurationSettings.GetConfigurationRootFactory internally to
           // get the currently set instance of IConfigurationRootFactory
           private readonly static IProperties properties = new Properties(ConfigurationSettings.GetConfigurationRootFactory().CreateConfiguration);
   
           /// <summary>
           /// Retrieves the value of a property from the current process.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <returns>The property value.</returns>
           public static string GetProperty(string key)
           {
               return properties.GetProperty(key);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process, 
           /// with a default value if it doens't exist or the caller doesn't have 
           /// permission to read the value.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist 
           /// or the caller doesn't have permission to read the value.</param>
           /// <returns>The property value.</returns>
           public static string GetProperty(string key, string defaultValue)
           {
               return properties.GetProperty(key, defaultValue);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process
           /// as <see cref="bool"/>. If the value cannot be cast to <see cref="bool"/>,
returns <c>false</c>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <returns>The property value.</returns>
           public static bool GetPropertyAsBoolean(string key)
           {
               return properties.GetPropertyAsBoolean(key);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process as <see cref="bool"/>,

           /// with a default value if it doens't exist, the caller doesn't have permission
to read the value, 
           /// or the value cannot be cast to a <see cref="bool"/>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist,
           /// the caller doesn't have permission to read the value, or the value cannot be
cast to <see cref="bool"/>.</param>
           /// <returns>The property value.</returns>
           public static bool GetPropertyAsBoolean(string key, bool defaultValue)
           {
               return properties.GetPropertyAsBoolean(key, defaultValue);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process
           /// as <see cref="int"/>. If the value cannot be cast to <see cref="int"/>,
returns <c>0</c>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <returns>The property value.</returns>
           public static int GetPropertyAsInt32(string key)
           {
               return properties.GetPropertyAsInt32(key);
           }
   
           /// <summary>
           /// Retrieves the value of a property from the current process as <see cref="int"/>,

           /// with a default value if it doens't exist, the caller doesn't have permission
to read the value, 
           /// or the value cannot be cast to a <see cref="int"/>.
           /// </summary>
           /// <param name="key">The name of the property.</param>
           /// <param name="defaultValue">The value to use if the property does not
exist,
           /// the caller doesn't have permission to read the value, or the value cannot be
cast to <see cref="int"/>.</param>
           /// <returns>The property value.</returns>
           public static int GetPropertyAsInt32(string key, int defaultValue)
           {
               return properties.GetPropertyAsInt32(key, defaultValue);
           }
       }
   ```




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