ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/efcore
File: src/ef/Commands/MigrationsRemoveCommand.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;

namespace Microsoft.EntityFrameworkCore.Tools.Commands;

// ReSharper disable once ArrangeTypeModifiers
internal partial class MigrationsRemoveCommand
{
    protected override int Execute(string[] args)
    {
        using var executor = CreateExecutor(args);
        var result = executor.RemoveMigration(Context!.Value(), _force!.HasValue(), _offline!.HasValue(), _connection!.Value());

        if (_json!.HasValue())
        {
            ReportJsonResults(result);
        }

        return base.Execute(args);
    }

    private static void ReportJsonResults(IDictionary result)
    {
        Reporter.WriteData("{");
        Reporter.WriteData("  \"migrationFile\": " + Json.Literal(result["MigrationFile"] as string) + ",");
        Reporter.WriteData("  \"metadataFile\": " + Json.Literal(result["MetadataFile"] as string) + ",");
        Reporter.WriteData("  \"snapshotFile\": " + Json.Literal(result["SnapshotFile"] as string));
        Reporter.WriteData("}");
    }
}


Review comment (bug):
`--offline` and `--force` can currently be specified together, but offline mode skips the applied-migration check and therefore cannot safely perform the revert behavior implied by `--force`. Add validation to reject this combination (or clearly define and document precedence) to avoid silently doing the wrong thing.
```suggestion
    {
        if (_offline!.HasValue() && _force!.HasValue())
        {
            Reporter.WriteError("The --offline and --force options cannot be used together.");
            return 1;
        }
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using Microsoft.EntityFrameworkCore.Tools.Properties;

namespace Microsoft.EntityFrameworkCore.Tools.Commands;

// ReSharper disable once ArrangeTypeModifiers
internal partial class MigrationsRemoveCommand
{
    protected override void Validate()
    {
        base.Validate();

        if (_offline!.HasValue() && _force!.HasValue())
        {
            throw new CommandException(Resources.OfflineForceConflict);
        }
    }

    protected override int Execute(string[] args)
    {
        using var executor = CreateExecutor(args);
        var result = executor.RemoveMigration(Context!.Value(), _force!.HasValue(), _offline!.HasValue(), _connection!.Value());

        if (_json!.HasValue())
        {
            ReportJsonResults(result);
        }

        return base.Execute(args);
    }

    private static void ReportJsonResults(IDictionary result)
    {
        Reporter.WriteData("{");
        Reporter.WriteData("  \"migrationFile\": " + Json.Literal(result["MigrationFile"] as string) + ",");
Repository: dotnet/efcore
File: src/ef/Commands/DatabaseDropCommand.cs (C#)

Code before:

using System;
using Microsoft.EntityFrameworkCore.Tools.Properties;

namespace Microsoft.EntityFrameworkCore.Tools.Commands;

// ReSharper disable once ArrangeTypeModifiers
internal partial class DatabaseDropCommand
{
    protected override int Execute(string[] args)
    {
        using var executor = CreateExecutor(args);

        void LogDropCommand(Func<object?, object?, string> resource)
        {
            if (_connection!.HasValue())
            {
                Reporter.WriteInformation("Warning: Using custom connection string. Database information cannot be displayed.");
            }
            else
            {
                var result = executor.GetContextInfo(Context!.Value());
                var databaseName = result["DatabaseName"] as string;
                var dataSource = result["DataSource"] as string;
                Reporter.WriteInformation(resource(databaseName, dataSource));
            }
        }

        if (_dryRun!.HasValue())
        {
            LogDropCommand(Resources.DatabaseDropDryRun);

            return 0;
        }

        if (!_force!.HasValue())
        {
            if (_connection!.HasValue())
            {
                Reporter.WriteInformation("Are you sure you want to drop the database specified by the connection string? (y/N)");
            }
            else
            {
                LogDropCommand(Resources.DatabaseDropPrompt);
            }
            
            var response = Console.ReadLine()!.Trim().ToUpperInvariant();
            if (response != "Y")
            {
                return 1;
            }

Review comment (suggestion):
@copilot Supply the connection string to `GetContextInfo` instead

Code after:

using System;
using Microsoft.EntityFrameworkCore.Tools.Properties;

namespace Microsoft.EntityFrameworkCore.Tools.Commands;

// ReSharper disable once ArrangeTypeModifiers
internal partial class DatabaseDropCommand
{
    protected override int Execute(string[] args)
    {
        using var executor = CreateExecutor(args);

        void LogDropCommand(Func<object?, object?, string> resource)
        {
            var result = executor.GetContextInfo(Context!.Value(), _connection!.Value());
            var databaseName = result["DatabaseName"] as string;
            var dataSource = result["DataSource"] as string;
            Reporter.WriteInformation(resource(databaseName, dataSource));
        }

        if (_dryRun!.HasValue())
        {
            LogDropCommand(Resources.DatabaseDropDryRun);

            return 0;
        }

        if (!_force!.HasValue())
        {
            LogDropCommand(Resources.DatabaseDropPrompt);
            var response = Console.ReadLine()!.Trim().ToUpperInvariant();
            if (response != "Y")
            {
                return 1;
            }
        }

        executor.DropDatabase(Context!.Value(), _connection!.Value());

        return base.Execute(args);
    }
}
Repository: dotnet/efcore
File: test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs (C#)

Code before:
﻿// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.EntityFrameworkCore.Query;

public class PrimitiveCollectionsQuerySqlServerTest : PrimitiveCollectionsQueryRelationalTestBase<
    PrimitiveCollectionsQuerySqlServerTest.PrimitiveCollectionsQuerySqlServerFixture>
{
    public override int? NumberOfValuesForHugeParameterCollectionTests { get; } = 5000;

    public PrimitiveCollectionsQuerySqlServerTest(PrimitiveCollectionsQuerySqlServerFixture fixture, ITestOutputHelper testOutputHelper)
        : base(fixture)
    {
        Fixture.TestSqlLoggerFactory.Clear();
        Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
    }

    public override async Task Inline_collection_of_ints_Contains()
    {
        await base.Inline_collection_of_ints_Contains();

        AssertSql(
            """
SELECT [p].[Id], [p].[Bool], [p].[Bools], [p].[DateTime], [p].[DateTimes], [p].[Enum], [p].[Enums], [p].[Int], [p].[Ints], [p].[NullableInt], [p].[NullableInts], [p].[NullableString], [p].[NullableStrings], [p].[NullableWrappedId], [p].[NullableWrappedIdWithNullableComparer], [p].[String], [p].[Strings], [p].[WrappedId]
FROM [PrimitiveCollectionsEntity] AS [p]
WHERE [p].[Int] IN (10, 999)

Review comment (suggestion):
The file appears to have gained a UTF-8 BOM (the invisible character before `// Licensed...`). The repo’s .editorconfig has `#charset = utf-8-bom` commented out, so adding BOMs is unexpected and creates noisy diffs; please re-save the file as UTF-8 *without* BOM.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.EntityFrameworkCore.Query;

public class PrimitiveCollectionsQuerySqlServerTest : PrimitiveCollectionsQueryRelationalTestBase<
    PrimitiveCollectionsQuerySqlServerTest.PrimitiveCollectionsQuerySqlServerFixture>
{
    public override int? NumberOfValuesForHugeParameterCollectionTests { get; } = 5000;

    public PrimitiveCollectionsQuerySqlServerTest(PrimitiveCollectionsQuerySqlServerFixture fixture, ITestOutputHelper testOutputHelper)
        : base(fixture)
    {
        Fixture.TestSqlLoggerFactory.Clear();
        Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
    }

    public override async Task Inline_collection_of_ints_Contains()
    {
        await base.Inline_collection_of_ints_Contains();

        AssertSql(
            """
SELECT [p].[Id], [p].[Bool], [p].[Bools], [p].[DateTime], [p].[DateTimes], [p].[Enum], [p].[Enums], [p].[Int], [p].[Ints], [p].[NullableInt], [p].[NullableInts], [p].[NullableString], [p].[NullableStrings], [p].[NullableWrappedId], [p].[NullableWrappedIdWithNullableComparer], [p].[String], [p].[Strings], [p].[WrappedId]
FROM [PrimitiveCollectionsEntity] AS [p]
WHERE [p].[Int] IN (10, 999)
Repository: dotnet/efcore
File: src/EFCore.SqlServer/Query/Internal/SqlServerSqlNullabilityProcessor.cs (C#)

Code before:
﻿// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Diagnostics.CodeAnalysis;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;

namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;

/// <summary>
///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
///     the same compatibility standards as public APIs. It may be changed or removed without notice in
///     any release. You should only use it directly in your code with extreme caution and knowing that
///     doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class SqlServerSqlNullabilityProcessor(
    RelationalParameterBasedSqlProcessorDependencies dependencies,
    RelationalParameterBasedSqlProcessorParameters parameters,
    ISqlServerSingletonOptions sqlServerSingletonOptions)
    : SqlNullabilityProcessor(dependencies, parameters)
{
    private const int MaxParameterCount = 2100 - 2;

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

Review comment (suggestion):
The file appears to have gained a UTF-8 BOM (the invisible character before `// Licensed...`). The repo’s .editorconfig has `#charset = utf-8-bom` commented out, so adding BOMs is unexpected and creates noisy diffs; please re-save the file as UTF-8 *without* BOM.

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections;
using System.Diagnostics.CodeAnalysis;
using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
using Microsoft.EntityFrameworkCore.SqlServer.Infrastructure.Internal;

namespace Microsoft.EntityFrameworkCore.SqlServer.Query.Internal;

/// <summary>
///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
///     the same compatibility standards as public APIs. It may be changed or removed without notice in
///     any release. You should only use it directly in your code with extreme caution and knowing that
///     doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class SqlServerSqlNullabilityProcessor(
    RelationalParameterBasedSqlProcessorDependencies dependencies,
    RelationalParameterBasedSqlProcessorParameters parameters,
    ISqlServerSingletonOptions sqlServerSingletonOptions)
    : SqlNullabilityProcessor(dependencies, parameters)
{
    private const int MaxParameterCount = 2100 - 2;

    /// <summary>
    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Repository: dotnet/efcore
File: test/EFCore.Relational.Tests/Update/CommandBatchPreparerTest.cs (C#)

Code before:
            .FirstOrDefault(cm => cm.ColumnName == "RowVersion");
        Assert.NotNull(rvModification);
        Assert.True(rvModification.IsCondition);
    }

    private abstract class EntityBase37169
    {
        public string Id { get; set; }
        public long RowVersion { get; set; }
    }

    private class OwnedEntity37169
    {
        public DateTime CreationDate { get; set; }
    }

    private class EntityA37169 : EntityBase37169
    {
        public bool SomeValue { get; set; }
        public OwnedEntity37169 Owned { get; set; }
    }

    private class EntityB37169 : EntityBase37169
    {
        public string Name { get; set; }
    }

    private class AnotherFakeEntity
    {
        public int Id { get; set; }
        public int? AnotherId { get; set; }
    }
}


Review comment (bug):
The new test/entities use the numeric suffix `37169`, but this PR is for issue #37588 (per PR metadata). Using the correct issue number in the test name and helper type names would improve traceability/searchability when debugging regressions.

Code after:
        // RowVersion should be a condition (used in WHERE clause)
        var rvModification = modifiedCommand.ColumnModifications
            .FirstOrDefault(cm => cm.ColumnName == "RowVersion");
        Assert.NotNull(rvModification);
        Assert.True(rvModification.IsCondition);
    }

    private abstract class EntityBase37588
    {
        public string Id { get; set; }
        public long RowVersion { get; set; }
    }

    private class OwnedEntity37588
    {
        public DateTime CreationDate { get; set; }
    }

    private class EntityA37588 : EntityBase37588
    {
        public bool SomeValue { get; set; }
        public OwnedEntity37588 Owned { get; set; }
    }

    private class EntityB37588 : EntityBase37588
    {
        public string Name { get; set; }
    }

    private class AnotherFakeEntity
    {
        public int Id { get; set; }
        public int? AnotherId { get; set; }
    }
}
Repository: dotnet/efcore
File: test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs (C#)

Code before:
""");
    }

    [ConditionalFact]
    public virtual void SqlOperation_handles_multiple_delimiters_in_string()
    {
        Generate(
            new SqlOperation { Sql = "SELECT '[bracket] and \"quote\"';" + EOL + "go" + EOL + "SELECT 2;" });

        AssertSql(
            """
SELECT '[bracket] and "quote"';
GO

SELECT 2;
""");
    }

    [ConditionalFact]
    public virtual void SqlOperation_handles_go_in_script_with_suppress_transaction()
    {
        Generate(
            new SqlOperation
            {
                Sql = "CREATE PROCEDURE dbo.Proc1 AS SELECT 1;" + EOL + "GO" + EOL
                    + "CREATE VIEW view1 AS SELECT 1 AS Id;" + EOL + "GO" + EOL + "SELECT 1;",
                SuppressTransaction = true
            },
            MigrationsSqlGenerationOptions.Script);

        AssertSql(
            """
CREATE PROCEDURE dbo.Proc1 AS SELECT 1;
GO

CREATE VIEW view1 AS SELECT 1 AS Id;
GO

SELECT 1;
""");
    }

    public override void InsertDataOperation_all_args_spatial()
    {
        base.InsertDataOperation_all_args_spatial();

        AssertSql(
            """
IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Full Name', N'Geometry') AND [object_id] = OBJECT_ID(N'[dbo].[People]'))
    SET IDENTITY_INSERT [dbo].[People] ON;
INSERT INTO [dbo].[People] ([Id], [Full Name], [Geometry])

Review comment (suggestion):
@copilot 
```suggestion
                    + "CREATE VIEW view1 AS SELECT 1 AS Id;" + EOL + "GO 2" + EOL + "SELECT 1;",
```

Code after:
""");
    }

    [ConditionalFact]
    public virtual void SqlOperation_handles_multiple_delimiters_in_string()
    {
        Generate(
            new SqlOperation { Sql = "SELECT '[bracket] and \"quote\"';" + EOL + "go" + EOL + "SELECT 2;" });

        AssertSql(
            """
SELECT '[bracket] and "quote"';
GO

SELECT 2;
""");
    }

    [ConditionalFact]
    public virtual void SqlOperation_handles_go_in_script_with_suppress_transaction()
    {
        Generate(
            new SqlOperation
            {
                Sql = "CREATE PROCEDURE dbo.Proc1 AS SELECT 1;" + EOL + "GO" + EOL
                    + "CREATE VIEW view1 AS SELECT 1 AS Id;" + EOL + "GO 2" + EOL + "SELECT 1;",
                SuppressTransaction = true
            },
            MigrationsSqlGenerationOptions.Script);

        AssertSql(
            """
CREATE PROCEDURE dbo.Proc1 AS SELECT 1;
GO
CREATE VIEW view1 AS SELECT 1 AS Id;
GO 2
SELECT 1;
""");
    }

    public override void InsertDataOperation_all_args_spatial()
    {
        base.InsertDataOperation_all_args_spatial();

        AssertSql(
            """
IF EXISTS (SELECT * FROM [sys].[identity_columns] WHERE [name] IN (N'Id', N'Full Name', N'Geometry') AND [object_id] = OBJECT_ID(N'[dbo].[People]'))
    SET IDENTITY_INSERT [dbo].[People] ON;
INSERT INTO [dbo].[People] ([Id], [Full Name], [Geometry])
VALUES (0, NULL, NULL),
(1, 'Daenerys Targaryen', NULL),
Repository: dotnet/maui
File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue25599.cs (C#)

Code before:
﻿#if ANDROID // Facing a full exception when clicking on the already selected tab(App.Tap()) in Windows, iOS, and macOS, so excluded those platforms.
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues
{
	public class Issue25599 : _IssuesUITest
	{
		public Issue25599(TestDevice testDevice) : base(testDevice){ }

		public override string Issue => "OnNavigating wrong target when tapping the same tab";

		[Test]
		[Category(UITestCategories.Navigation)]
		public void NavigatingEventFired()
		{
			App.WaitForElement("HomePageButton");
			App.Tap("HomePageButton");
			App.WaitForElement("DetailsPageLabel");
			App.Tap("Home"); // Tapping already selected Home tab
			App.WaitForElement("DetailsPageLabel");
			App.WaitForNoElement("HomePageLabel"); // Navigation does not occur when clicking on an already selected tab
		}
	}
}

Review comment (bug):
Could you share the exception? Want to determinate if is related with a possible bug on Android or if we need to review something at testing stuff.

Code after:
﻿#if TEST_FAILS_ON_CATALYST && TEST_FAILS_ON_IOS // To fix the issue in ios https://github.com/dotnet/maui/pull/25749#pullrequestreview-2554186362
using NUnit.Framework;
using UITest.Appium;
using UITest.Core;

namespace Microsoft.Maui.TestCases.Tests.Issues
{
	public class Issue25599 : _IssuesUITest
	{
		public Issue25599(TestDevice testDevice) : base(testDevice) { }

		public override string Issue => "OnNavigating wrong target when tapping the same tab";

		[Test]
		[Category(UITestCategories.Navigation)]
		public void NavigatingEventFired()
		{
			App.WaitForElement("HomePageButton");
			App.Tap("HomePageButton");
			App.WaitForElement("DetailsPageLabel");
			App.Tap("Home"); // Tapping already selected tab using Title
			App.WaitForElement("DetailsPageLabel");
			App.WaitForNoElement("HomePageLabel"); // Navigation does not occur when clicking on an already selected tab
		}
	}
}
Repository: dotnet/maui
File: src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs (C#)

Code before:
			{
				UpdatePresented(false, true);
			});

			if (FlyoutOverlapsDetailsInPopoverMode)
				_tapGesture.ShouldReceiveTouch = shouldReceive;

			_clickOffView.AddGestureRecognizer(_tapGesture);

			PackContainers();
			UpdateFlyoutPageContainers();

			UpdateBackground();

			UpdatePanGesture();
			UpdateApplyShadow(((FlyoutPage)Element).OnThisPlatform().GetApplyShadow());
			UpdatePageSpecifics();
		}

		public override void ViewWillTransitionToSize(CoreGraphics.CGSize toSize, IUIViewControllerTransitionCoordinator coordinator)
		{
			base.ViewWillTransitionToSize(toSize, coordinator);

			if (FlyoutOverlapsDetailsInPopoverMode)
			{
				if (FlyoutPageController.ShouldShowSplitMode || Presented)
					UpdatePresented(true);
				else
					UpdatePresented(false);
			}
			else
			{
				if (!FlyoutPageController.ShouldShowSplitMode && _presented)
					UpdatePresented(false);
			}

			UpdateLeftBarButton();
		}

		void UpdatePresented(bool newValue, bool animated = false)
		{
			if (Presented == newValue)
			{
				UpdateClickOffView();
				return;
			}

			if (!newValue && FlyoutPageController.ShouldShowSplitMode)
			{
				return;
			}

Review comment (suggestion):
I think this causes the flyout to open in too many scenarios.

For example, if I'm on an ipad now and I rotate to landscape and then back to portrait the flyout will remain open. 

we probably need to condition on the type of transition that's happening here so this code doesn't run when it's just related to a size change

Code after:
			{
				UpdatePresented(false, true);
			});

			if (FlyoutOverlapsDetailsInPopoverMode)
				_tapGesture.ShouldReceiveTouch = shouldReceive;

			_clickOffView.AddGestureRecognizer(_tapGesture);

			PackContainers();
			UpdateFlyoutPageContainers();

			UpdateBackground();

			UpdatePanGesture();
			UpdateApplyShadow(((FlyoutPage)Element).OnThisPlatform().GetApplyShadow());
			UpdatePageSpecifics();
		}

		public override void ViewWillTransitionToSize(CoreGraphics.CGSize toSize, IUIViewControllerTransitionCoordinator coordinator)
		{
			base.ViewWillTransitionToSize(toSize, coordinator);
			if (!OperatingSystem.IsMacCatalyst())
			{
				bool shouldShowSplitMode = FlyoutPageController.ShouldShowSplitMode;
				if (FlyoutOverlapsDetailsInPopoverMode)
				{
					UpdatePresented(shouldShowSplitMode);
				}
				else if (!shouldShowSplitMode && _presented)
				{
					UpdatePresented(false);
				}
			}

			UpdateLeftBarButton();
		}

		void UpdatePresented(bool newValue, bool animated = false)
		{
			if (Presented == newValue)
			{
				UpdateClickOffView();
				return;
			}

			if (!newValue && FlyoutPageController.ShouldShowSplitMode)
			{
				return;
			}