From fc9ff223d650b2f903a6216cdbb7a2303a65d27f Mon Sep 17 00:00:00 2001
From: Austin Drenski <austin.drenski@gmail.com>
Date: Tue, 15 May 2018 12:51:51 -0400
Subject: [PATCH] Addressing code review comments #2 (2018-05-15).

---
 .../Internal/NpgsqlRangeTranslator.cs         |  49 +---
 .../Query/RangeQueryNpgsqlFixture.cs          | 158 ------------
 .../Query/RangeQueryNpgsqlTest.cs             | 244 +++++++++++++++---
 3 files changed, 219 insertions(+), 232 deletions(-)
 delete mode 100644 test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlFixture.cs

diff --git a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs
index 30abcf839..39127c444 100644
--- a/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs
+++ b/src/EFCore.PG/Query/ExpressionTranslators/Internal/NpgsqlRangeTranslator.cs
@@ -23,7 +23,6 @@
 
 #endregion
 
-using System;
 using System.Linq.Expressions;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Query.ExpressionTranslators;
@@ -41,70 +40,46 @@ public class NpgsqlRangeTranslator : IMethodCallTranslator
     {
         /// <inheritdoc />
         [CanBeNull]
-        public Expression Translate(MethodCallExpression methodCallExpression) =>
-            TryTranslateOperator(methodCallExpression);
-
-        /// <summary>
-        /// Attempts to translate the <see cref="MethodCallExpression"/> as a PostgreSQL range operator.
-        /// </summary>
-        /// <param name="expression">The <see cref="MethodCallExpression"/> to be translated.</param>
-        /// <returns>
-        /// The expression if successful; otherwise, null.
-        /// </returns>
-        [CanBeNull]
-        static Expression TryTranslateOperator([NotNull] MethodCallExpression expression)
+        public Expression Translate(MethodCallExpression expression)
         {
             switch (expression.Method.Name)
             {
             case nameof(NpgsqlRangeExtensions.Contains):
-                return MakeBinaryExpression(expression, "@>", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "@>", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.ContainedBy):
-                return MakeBinaryExpression(expression, "<@", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "<@", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.Overlaps):
-                return MakeBinaryExpression(expression, "&&", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&&", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.IsStrictlyLeftOf):
-                return MakeBinaryExpression(expression, "<<", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "<<", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.IsStrictlyRightOf):
-                return MakeBinaryExpression(expression, ">>", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], ">>", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.DoesNotExtendRightOf):
-                return MakeBinaryExpression(expression, "&<", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&<", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.DoesNotExtendLeftOf):
-                return MakeBinaryExpression(expression, "&>", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "&>", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.IsAdjacentTo):
-                return MakeBinaryExpression(expression, "-|-", typeof(bool));
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "-|-", typeof(bool));
 
             case nameof(NpgsqlRangeExtensions.Union):
-                return MakeBinaryExpression(expression, "+", expression.Arguments[0].Type);
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "+", expression.Arguments[0].Type);
 
             case nameof(NpgsqlRangeExtensions.Intersect):
-                return MakeBinaryExpression(expression, "*", expression.Arguments[0].Type);
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "*", expression.Arguments[0].Type);
 
             case nameof(NpgsqlRangeExtensions.Except):
-                return MakeBinaryExpression(expression, "-", expression.Arguments[0].Type);
+                return new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], "-", expression.Arguments[0].Type);
 
             default:
                 return null;
             }
         }
-
-        /// <summary>
-        /// Constructs a <see cref="CustomBinaryExpression"/>.
-        /// </summary>
-        /// <param name="expression">The <see cref="MethodCallExpression"/> containing two parameters.</param>
-        /// <param name="symbol">The symbolic operator for PostgreSQL.</param>
-        /// <param name="returnType">The return type of the operator.</param>
-        /// <returns>
-        /// A <see cref="CustomBinaryExpression"/>.
-        /// </returns>
-        [NotNull]
-        static Expression MakeBinaryExpression([NotNull] MethodCallExpression expression, [NotNull] string symbol, [NotNull] Type returnType) =>
-            new CustomBinaryExpression(expression.Arguments[0], expression.Arguments[1], symbol, returnType);
     }
 }
diff --git a/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlFixture.cs b/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlFixture.cs
deleted file mode 100644
index 9ee91b1e8..000000000
--- a/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlFixture.cs
+++ /dev/null
@@ -1,158 +0,0 @@
-using System;
-using Microsoft.EntityFrameworkCore;
-using Microsoft.EntityFrameworkCore.TestUtilities;
-using Microsoft.Extensions.DependencyInjection;
-using Microsoft.Extensions.Logging;
-using Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities;
-using NpgsqlTypes;
-
-namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query
-{
-    /// <summary>
-    /// Represents a fixture suitable for testing range operators.
-    /// </summary>
-    public class RangeQueryNpgsqlFixture : IDisposable
-    {
-        /// <summary>
-        /// The <see cref="NpgsqlTestStore"/> used for testing.
-        /// </summary>
-        private readonly NpgsqlTestStore _testStore;
-
-        /// <summary>
-        /// The <see cref="DbContextOptions"/> used for testing.
-        /// </summary>
-        private readonly DbContextOptions _options;
-
-        /// <summary>
-        /// The logger factory used for testing.
-        /// </summary>
-        public TestSqlLoggerFactory TestSqlLoggerFactory { get; }
-
-        /// <summary>
-        /// Initializes a <see cref="RangeQueryNpgsqlFixture"/>.
-        /// </summary>
-        // ReSharper disable once UnusedMember.Global
-        public RangeQueryNpgsqlFixture()
-        {
-            TestSqlLoggerFactory = new TestSqlLoggerFactory();
-
-            _testStore = NpgsqlTestStore.CreateScratch();
-
-            _options =
-                new DbContextOptionsBuilder()
-                    .UseNpgsql(_testStore.ConnectionString, b => b.ApplyConfiguration())
-                    .UseInternalServiceProvider(
-                        new ServiceCollection()
-                            .AddEntityFrameworkNpgsql()
-                            .AddSingleton<ILoggerFactory>(TestSqlLoggerFactory)
-                            .BuildServiceProvider())
-                    .Options;
-
-            using (RangeContext context = CreateContext())
-            {
-                context.Database.EnsureCreated();
-
-                context.RangeTestEntities.AddRange(
-                    new RangeTestEntity
-                    {
-                        Id = 1,
-                        // (0, 10)
-                        Range = new NpgsqlRange<int>(0, false, false, 10, false, false),
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 2,
-                        // [0, 10)
-                        Range = new NpgsqlRange<int>(0, true, false, 10, false, false)
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 3,
-                        // [0, 10]
-                        Range = new NpgsqlRange<int>(0, true, false, 10, true, false)
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 4,
-                        // [0, ∞)
-                        Range = new NpgsqlRange<int>(0, true, false, 0, false, true)
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 5,
-                        // (-∞, 10]
-                        Range = new NpgsqlRange<int>(0, false, true, 10, true, false)
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 6,
-                        // (-∞, ∞)
-                        Range = new NpgsqlRange<int>(0, false, true, 0, false, true)
-                    },
-                    new RangeTestEntity
-                    {
-                        Id = 7,
-                        // (-∞, ∞)
-                        Range = new NpgsqlRange<int>(0, false, true, 0, false, true)
-                    });
-
-                context.SaveChanges();
-            }
-        }
-
-        /// <summary>
-        /// Creates a new <see cref="RangeContext"/>.
-        /// </summary>
-        /// <returns>
-        /// A <see cref="RangeContext"/> for testing.
-        /// </returns>
-        public RangeContext CreateContext()
-        {
-            return new RangeContext(_options);
-        }
-
-        /// <inheritdoc />
-        public void Dispose()
-        {
-            _testStore.Dispose();
-        }
-    }
-
-    /// <summary>
-    /// Represents an entity suitable for testing range operators.
-    /// </summary>
-    public class RangeTestEntity
-    {
-        /// <summary>
-        /// The primary key.
-        /// </summary>
-        public int Id { get; set; }
-
-        /// <summary>
-        /// The range of integers.
-        /// </summary>
-        public NpgsqlRange<int> Range { get; set; }
-    }
-
-    /// <summary>
-    /// Represents a database suitable for testing range operators.
-    /// </summary>
-    public class RangeContext : DbContext
-    {
-        /// <summary>
-        /// Represents a set of entities with <see cref="NpgsqlRange{T}"/> properties.
-        /// </summary>
-        public DbSet<RangeTestEntity> RangeTestEntities { get; set; }
-
-        /// <summary>
-        /// Initializes a <see cref="RangeContext"/>.
-        /// </summary>
-        /// <param name="options">
-        /// The options to be used for configuration.
-        /// </param>
-        public RangeContext(DbContextOptions options) : base(options) { }
-
-        /// <inheritdoc />
-        protected override void OnModelCreating(ModelBuilder builder) { }
-    }
-}
diff --git a/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs b/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs
index b70a11676..c31d32016 100644
--- a/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs
+++ b/test/EFCore.PG.FunctionalTests/Query/RangeQueryNpgsqlTest.cs
@@ -1,5 +1,11 @@
-using System.Collections.Generic;
+using System;
+using System.Collections.Generic;
 using System.Linq;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Npgsql.EntityFrameworkCore.PostgreSQL.TestUtilities;
 using NpgsqlTypes;
 using Xunit;
 
@@ -8,40 +14,13 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Query
     /// <summary>
     /// Provides unit tests for range operator translations.
     /// </summary>
-    public class RangeQueryNpgsqlTest : IClassFixture<RangeQueryNpgsqlFixture>
+    public class RangeQueryNpgsqlTest : IClassFixture<RangeQueryNpgsqlTest.RangeQueryNpgsqlFixture>
     {
         /// <summary>
         /// Provides resources for unit tests.
         /// </summary>
         RangeQueryNpgsqlFixture Fixture { get; }
 
-        /// <summary>
-        /// Provides theory data for integers.
-        /// </summary>
-        public static IEnumerable<object[]> IntegerTheoryData => Enumerable.Range(-10, 10).Select(x => new object[] { x });
-
-        /// <summary>
-        /// Provides theory data for ranges.
-        /// </summary>
-        public static IEnumerable<object[]> RangeTheoryData =>
-            new List<object[]>
-            {
-                // (0,5)
-                new object[] { new NpgsqlRange<int>(0, false, false, 5, false, false) },
-                // [0,5]
-                new object[] { new NpgsqlRange<int>(0, true, false, 5, true, false) },
-                // (,)
-                new object[] { new NpgsqlRange<int>(0, false, true, 0, false, true) },
-                // (,)
-                new object[] { new NpgsqlRange<int>(0, false, true, 5, false, true) },
-                // (0,)
-                new object[] { new NpgsqlRange<int>(0, false, false, 0, false, true) },
-                // (0,)
-                new object[] { new NpgsqlRange<int>(0, false, false, 5, false, true) },
-                // (,5)
-                new object[] { new NpgsqlRange<int>(0, false, true, 5, false, false) }
-            };
-
         /// <summary>
         /// Initializes resources for unit tests.
         /// </summary>
@@ -52,14 +31,7 @@ public RangeQueryNpgsqlTest(RangeQueryNpgsqlFixture fixture)
             Fixture.TestSqlLoggerFactory.Clear();
         }
 
-        /// <summary>
-        /// Asserts that the SQL fragment appears in the logs.
-        /// </summary>
-        /// <param name="sql">The SQL statement or fragment to search for in the logs.</param>
-        public void AssertContainsSql(string sql)
-        {
-            Assert.Contains(sql, Fixture.TestSqlLoggerFactory.Sql);
-        }
+        #region Tests
 
         /// <summary>
         /// Tests translation for <see cref="NpgsqlRangeExtensions.Contains{T}(NpgsqlRange{T},NpgsqlRange{T})"/>.
@@ -517,5 +489,203 @@ public void RangeExceptRange(NpgsqlRange<int> range)
                 AssertContainsSql("SELECT x.\"Range\" - @__range_0");
             }
         }
+
+        #endregion Tests
+
+        #region TheoryData
+
+        /// <summary>
+        /// Provides theory data for integers.
+        /// </summary>
+        public static IEnumerable<object[]> IntegerTheoryData => Enumerable.Range(-10, 10).Select(x => new object[] { x });
+
+        /// <summary>
+        /// Provides theory data for ranges.
+        /// </summary>
+        public static IEnumerable<object[]> RangeTheoryData =>
+            new List<object[]>
+            {
+                // (0,5)
+                new object[] { new NpgsqlRange<int>(0, false, false, 5, false, false) },
+                // [0,5]
+                new object[] { new NpgsqlRange<int>(0, true, false, 5, true, false) },
+                // (,)
+                new object[] { new NpgsqlRange<int>(0, false, true, 0, false, true) },
+                // (,)
+                new object[] { new NpgsqlRange<int>(0, false, true, 5, false, true) },
+                // (0,)
+                new object[] { new NpgsqlRange<int>(0, false, false, 0, false, true) },
+                // (0,)
+                new object[] { new NpgsqlRange<int>(0, false, false, 5, false, true) },
+                // (,5)
+                new object[] { new NpgsqlRange<int>(0, false, true, 5, false, false) }
+            };
+
+        #endregion
+
+        #region Fixtures
+
+        /// <summary>
+        /// Represents a fixture suitable for testing range operators.
+        /// </summary>
+        public class RangeQueryNpgsqlFixture : IDisposable
+        {
+            /// <summary>
+            /// The <see cref="NpgsqlTestStore"/> used for testing.
+            /// </summary>
+            private readonly NpgsqlTestStore _testStore;
+
+            /// <summary>
+            /// The <see cref="DbContextOptions"/> used for testing.
+            /// </summary>
+            private readonly DbContextOptions _options;
+
+            /// <summary>
+            /// The logger factory used for testing.
+            /// </summary>
+            public TestSqlLoggerFactory TestSqlLoggerFactory { get; }
+
+            /// <summary>
+            /// Initializes a <see cref="RangeQueryNpgsqlFixture"/>.
+            /// </summary>
+            // ReSharper disable once UnusedMember.Global
+            public RangeQueryNpgsqlFixture()
+            {
+                TestSqlLoggerFactory = new TestSqlLoggerFactory();
+
+                _testStore = NpgsqlTestStore.CreateScratch();
+
+                _options =
+                    new DbContextOptionsBuilder()
+                        .UseNpgsql(_testStore.ConnectionString, b => b.ApplyConfiguration())
+                        .UseInternalServiceProvider(
+                            new ServiceCollection()
+                                .AddEntityFrameworkNpgsql()
+                                .AddSingleton<ILoggerFactory>(TestSqlLoggerFactory)
+                                .BuildServiceProvider())
+                        .Options;
+
+                using (RangeContext context = CreateContext())
+                {
+                    context.Database.EnsureCreated();
+
+                    context.RangeTestEntities.AddRange(
+                        new RangeTestEntity
+                        {
+                            Id = 1,
+                            // (0, 10)
+                            Range = new NpgsqlRange<int>(0, false, false, 10, false, false),
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 2,
+                            // [0, 10)
+                            Range = new NpgsqlRange<int>(0, true, false, 10, false, false)
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 3,
+                            // [0, 10]
+                            Range = new NpgsqlRange<int>(0, true, false, 10, true, false)
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 4,
+                            // [0, ∞)
+                            Range = new NpgsqlRange<int>(0, true, false, 0, false, true)
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 5,
+                            // (-∞, 10]
+                            Range = new NpgsqlRange<int>(0, false, true, 10, true, false)
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 6,
+                            // (-∞, ∞)
+                            Range = new NpgsqlRange<int>(0, false, true, 0, false, true)
+                        },
+                        new RangeTestEntity
+                        {
+                            Id = 7,
+                            // (-∞, ∞)
+                            Range = new NpgsqlRange<int>(0, false, true, 0, false, true)
+                        });
+
+                    context.SaveChanges();
+                }
+            }
+
+            /// <summary>
+            /// Creates a new <see cref="RangeContext"/>.
+            /// </summary>
+            /// <returns>
+            /// A <see cref="RangeContext"/> for testing.
+            /// </returns>
+            public RangeContext CreateContext()
+            {
+                return new RangeContext(_options);
+            }
+
+            /// <inheritdoc />
+            public void Dispose()
+            {
+                _testStore.Dispose();
+            }
+        }
+
+        /// <summary>
+        /// Represents an entity suitable for testing range operators.
+        /// </summary>
+        public class RangeTestEntity
+        {
+            /// <summary>
+            /// The primary key.
+            /// </summary>
+            public int Id { get; set; }
+
+            /// <summary>
+            /// The range of integers.
+            /// </summary>
+            public NpgsqlRange<int> Range { get; set; }
+        }
+
+        /// <summary>
+        /// Represents a database suitable for testing range operators.
+        /// </summary>
+        public class RangeContext : DbContext
+        {
+            /// <summary>
+            /// Represents a set of entities with <see cref="NpgsqlRange{T}"/> properties.
+            /// </summary>
+            public DbSet<RangeTestEntity> RangeTestEntities { get; set; }
+
+            /// <summary>
+            /// Initializes a <see cref="RangeContext"/>.
+            /// </summary>
+            /// <param name="options">
+            /// The options to be used for configuration.
+            /// </param>
+            public RangeContext(DbContextOptions options) : base(options) { }
+
+            /// <inheritdoc />
+            protected override void OnModelCreating(ModelBuilder builder) { }
+        }
+
+        #endregion
+
+        #region Helpers
+
+        /// <summary>
+        /// Asserts that the SQL fragment appears in the logs.
+        /// </summary>
+        /// <param name="sql">The SQL statement or fragment to search for in the logs.</param>
+        public void AssertContainsSql(string sql)
+        {
+            Assert.Contains(sql, Fixture.TestSqlLoggerFactory.Sql);
+        }
+
+        #endregion
     }
 }