ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/aspnetcore
File: src/SignalR/clients/ts/signalr/src/HubConnection.ts (TypeScript)

Code before:
            item,
            type: MessageType.StreamItem,
        };
    }

    private _createCompletionMessage(id: string, error?: any, result?: any): CompletionMessage {
        if (error) {
            return {
                error,
                invocationId: id,
                type: MessageType.Completion,
            };
        }

        return {
            invocationId: id,
            result,
            type: MessageType.Completion,
        };
    }

    private _createCloseMessage(): CloseMessage {
        return { type: MessageType.Close };
    }

    private _trySendPingMessage(): Promise<void> {
        try {
            await this._sendMessage(this._cachedPingMessage);
        } catch {
            // We don't care about the error. It should be seen elsewhere in the client.
            // The connection is probably in a bad or closed state now, cleanup the timer so it stops triggering
            this._cleanupPingTimer();
        }
    }
}


Review comment (suggestion):
```suggestion
    private async _trySendPingMessage(): Promise<void> {
```

Code after:
            item,
            type: MessageType.StreamItem,
        };
    }

    private _createCompletionMessage(id: string, error?: any, result?: any): CompletionMessage {
        if (error) {
            return {
                error,
                invocationId: id,
                type: MessageType.Completion,
            };
        }

        return {
            invocationId: id,
            result,
            type: MessageType.Completion,
        };
    }

    private _createCloseMessage(): CloseMessage {
        return { type: MessageType.Close };
    }

    private async _trySendPingMessage(): Promise<void> {
        try {
            await this._sendMessage(this._cachedPingMessage);
        } catch {
            // We don't care about the error. It should be seen elsewhere in the client.
            // The connection is probably in a bad or closed state now, cleanup the timer so it stops triggering
            this._cleanupPingTimer();
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/Components/src/ComponentsMetrics.cs (C#)

Code before:
    {
        if (route != null)
        {
            tags.Add("aspnetcore.components.route", route);
        }
    }

    private static void AddMethodNameTag(ref TagList tags, string? methodName)
    {
        if (methodName != null)
        {
            tags.Add("code.function.name", methodName);
        }
    }

    private static void AddAttributeNameTag(ref TagList tags, string? attributeName)
    {
        if (attributeName != null)
        {
            tags.Add("aspnetcore.components.attribute.name", attributeName);
        }
    }

    private static void AddErrorTag(ref TagList tags, Exception? exception)
    {
        if (exception?.GetType().FullName != null)
        {
            tags.Add("error.type", exception.GetType().FullName);
        }
    }
}


Review comment (suggestion):
perhaps add local variable for the `exception?.GetType().FullName` expression

Code after:
    {
        if (route != null)
        {
            tags.Add("aspnetcore.components.route", route);
        }
    }

    private static void AddMethodNameTag(ref TagList tags, string? methodName)
    {
        if (methodName != null)
        {
            tags.Add("code.function.name", methodName);
        }
    }

    private static void AddAttributeNameTag(ref TagList tags, string? attributeName)
    {
        if (attributeName != null)
        {
            tags.Add("aspnetcore.components.attribute.name", attributeName);
        }
    }

    private static void AddErrorTag(ref TagList tags, Exception? exception)
    {
        var errorType = exception?.GetType().FullName;
        if (errorType is not null)
        {
            tags.Add("error.type", errorType);
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Components/Components/src/ComponentsMetrics.cs (C#)

Code before:
            tags.Add("aspnetcore.components.route", route);
        }
    }

    private static void AddMethodNameTag(ref TagList tags, string? methodName)
    {
        if (methodName != null)
        {
            tags.Add("code.function.name", methodName);
        }
    }

    private static void AddAttributeNameTag(ref TagList tags, string? attributeName)
    {
        if (attributeName != null)
        {
            tags.Add("aspnetcore.components.attribute.name", attributeName);
        }
    }

    private static void AddErrorTag(ref TagList tags, Exception? exception)
    {
        if (exception?.GetType().FullName != null)
        {
            tags.Add("error.type", exception.GetType().FullName);
        }
    }
}


Review comment (suggestion):
```suggestion
        if (exception?.GetType().FullName is { } errorType)
        {
            tags.Add("error.type", errorType);
        }
```

Code after:
            tags.Add("aspnetcore.components.route", route);
        }
    }

    private static void AddMethodNameTag(ref TagList tags, string? methodName)
    {
        if (methodName != null)
        {
            tags.Add("code.function.name", methodName);
        }
    }

    private static void AddAttributeNameTag(ref TagList tags, string? attributeName)
    {
        if (attributeName != null)
        {
            tags.Add("aspnetcore.components.attribute.name", attributeName);
        }
    }

    private static void AddErrorTag(ref TagList tags, Exception? exception)
    {
        var errorType = exception?.GetType().FullName;
        if (errorType is not null)
        {
            tags.Add("error.type", errorType);
        }
    }
}
Repository: dotnet/aspnetcore
File: src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.IValidatableObject.cs (C#)

Code before:
            }
        });
    }

    [Fact]
    public async Task CanValidateIValidatableObject_WithoutPropertyValidations()
    {
        var source = """
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Validation;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

builder.Services.AddValidation();

WebApplication app = builder. Build();

app.MapPost("/base", (BaseClass model) => Results.Ok(model));
app.MapPost("/derived", (DerivedClass model) => Results.Ok(model));

app.Run();

public class BaseClass : IValidatableObject
{
    public string? Value { get; set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        if (string.IsNullOrEmpty(Value))
        {
            yield return new ValidationResult("Value cannot be null or empty.", [nameof(Value)]);
        }
    }
}

public class DerivedClass : BaseClass
{
}
""";

        await Verify(source, out var compilation);

        await VerifyEndpoint(compilation, "/base", async (endpoint, serviceProvider) =>
        {

Review comment (suggestion):
We should add a case that validates that a class with a property that only implements IValidatableObject also works. I don't anticipate that this will require any code changes since the recursion already handles that.

Code after:
            }
        });
    }

    [Fact]
    public async Task CanValidateIValidatableObject_WithoutPropertyValidations()
    {
        var source = """
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Validation;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection;

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

builder.Services.AddValidation();

WebApplication app = builder. Build();

app.MapPost("/base", (BaseClass model) => Results.Ok(model));
app.MapPost("/derived", (DerivedClass model) => Results.Ok(model));
app.MapPost("/complex", (ComplexClass model) => Results.Ok(model));

app.Run();

public class BaseClass : IValidatableObject
{
    public string? Value { get; set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        if (string.IsNullOrEmpty(Value))
        {
            yield return new ValidationResult("Value cannot be null or empty.", [nameof(Value)]);
        }
    }
}

public class DerivedClass : BaseClass
{
}

public class ComplexClass
{
    public NestedClass? NestedObject { get; set; }
}
Repository: dotnet/aspnetcore
File: src/Shared/Obsoletions.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.Generic;
using System.Diagnostics;

namespace Microsoft.AspNetCore.Shared;

internal sealed class Obsoletions
{
    internal const string RuntimeSharedUrlFormat = "https://aka.ms/dotnet-warnings/{0}";
    internal const string AspNetCoreSharedUrlFormat = "https://aka.ms/aspnet/deprecate/{0}";

    internal const string RuntimeTlsCipherAlgorithmEnumsMessage = "KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherStrength, HashAlgorithm and HashStrength properties are obsolete. Use NegotiatedCipherSuite instead.";
    internal const string RuntimeTlsCipherAlgorithmEnumsDiagId = "SYSLIB0058";
}


Review comment (suggestion):
@copilot It doesn't look like you are using this shared constant anywhere though. Can you fix that by making sure everywhere you added a UrlFormat it references this constant as opposed to duplicating this string?

Also, make sure that you also look for Obsolete attribute declarations that span multiple lines, as there should be some instances of those in the repo as well. Make sure that your search to ensure every ASPDEPR diagnostic has a URLFormat is extensive.

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.Generic;
using System.Diagnostics;

namespace Microsoft.AspNetCore.Shared;

internal sealed class Obsoletions
{
    internal const string RuntimeSharedUrlFormat = "https://aka.ms/dotnet-warnings/{0}";

    internal const string RuntimeTlsCipherAlgorithmEnumsMessage = "KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherStrength, HashAlgorithm and HashStrength properties are obsolete. Use NegotiatedCipherSuite instead.";
    internal const string RuntimeTlsCipherAlgorithmEnumsDiagId = "SYSLIB0058";

    // ASP.NET Core deprecated API URLs (not using {0} placeholder - these are explicit URLs)
    internal const string AspNetCoreDeprecate002Url = "https://aka.ms/aspnet/deprecate/002";
    internal const string AspNetCoreDeprecate003Url = "https://aka.ms/aspnet/deprecate/003";
    internal const string AspNetCoreDeprecate004Url = "https://aka.ms/aspnet/deprecate/004";
    internal const string AspNetCoreDeprecate005Url = "https://aka.ms/aspnet/deprecate/005";
    internal const string AspNetCoreDeprecate006Url = "https://aka.ms/aspnet/deprecate/006";
    internal const string AspNetCoreDeprecate008Url = "https://aka.ms/aspnet/deprecate/008";
    internal const string AspNetCoreDeprecate009Url = "https://aka.ms/aspnet/deprecate/009";
}
Repository: dotnet/aspnetcore
File: src/Shared/Obsoletions.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.Generic;
using System.Diagnostics;

namespace Microsoft.AspNetCore.Shared;

internal sealed class Obsoletions
{
    internal const string RuntimeSharedUrlFormat = "https://aka.ms/dotnet-warnings/{0}";
    internal const string AspNetCoreSharedUrlFormat = "https://aka.ms/aspnet/deprecate/{0}";

    internal const string RuntimeTlsCipherAlgorithmEnumsMessage = "KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherStrength, HashAlgorithm and HashStrength properties are obsolete. Use NegotiatedCipherSuite instead.";
    internal const string RuntimeTlsCipherAlgorithmEnumsDiagId = "SYSLIB0058";
}


Review comment (suggestion):
This will end up being `https://aka.ms/aspnet/deprecate/aspdepr003`? Not `https://aka.ms/aspnet/deprecate/003`

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.Generic;
using System.Diagnostics;

namespace Microsoft.AspNetCore.Shared;

internal sealed class Obsoletions
{
    internal const string RuntimeSharedUrlFormat = "https://aka.ms/dotnet-warnings/{0}";

    internal const string RuntimeTlsCipherAlgorithmEnumsMessage = "KeyExchangeAlgorithm, KeyExchangeStrength, CipherAlgorithm, CipherStrength, HashAlgorithm and HashStrength properties are obsolete. Use NegotiatedCipherSuite instead.";
    internal const string RuntimeTlsCipherAlgorithmEnumsDiagId = "SYSLIB0058";

    // ASP.NET Core deprecated API URLs (not using {0} placeholder - these are explicit URLs)
    internal const string AspNetCoreDeprecate002Url = "https://aka.ms/aspnet/deprecate/002";
    internal const string AspNetCoreDeprecate003Url = "https://aka.ms/aspnet/deprecate/003";
    internal const string AspNetCoreDeprecate004Url = "https://aka.ms/aspnet/deprecate/004";
    internal const string AspNetCoreDeprecate005Url = "https://aka.ms/aspnet/deprecate/005";
    internal const string AspNetCoreDeprecate006Url = "https://aka.ms/aspnet/deprecate/006";
    internal const string AspNetCoreDeprecate008Url = "https://aka.ms/aspnet/deprecate/008";
    internal const string AspNetCoreDeprecate009Url = "https://aka.ms/aspnet/deprecate/009";
}
Repository: dotnet/aspnetcore
File: src/Shared/test/Shared.Tests/StackTraceHelperTest.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;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Extensions.StackTrace.Sources;
using ThrowingLibrary;
using Xunit;

namespace Microsoft.Extensions.Internal;

public class StackTraceHelperTest
{
    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
    [Fact]
    public void StackTraceHelper_IncludesLineNumbersForFiles()
    {
        // Arrange
        Exception exception = null;
        try
        {
            // Throwing an exception in the current assembly always seems to populate the full stack
            // trace regardless of symbol type. Crossing assembly boundaries ensures PortablePdbReader gets used
            // on desktop.
            Thrower.Throw();
        }
        catch (Exception ex)
        {
            exception = ex;
        }

        // Act
        var stackFrames = StackTraceHelper.GetFrames(exception, out _);

        // Assert
        Assert.Collection(stackFrames,
            frame =>
            {
                Assert.Contains("Thrower.cs", frame.FilePath);

Review comment (suggestion):
```suggestion

    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
```

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;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Extensions.StackTrace.Sources;
using ThrowingLibrary;
using Xunit;

namespace Microsoft.Extensions.Internal;

public class StackTraceHelperTest
{
    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
    
    [Fact]
    public void StackTraceHelper_IncludesLineNumbersForFiles()
    {
        // Arrange
        Exception exception = null;
        try
        {
            // Throwing an exception in the current assembly always seems to populate the full stack
            // trace regardless of symbol type. Crossing assembly boundaries ensures PortablePdbReader gets used
            // on desktop.
            Thrower.Throw();
        }
        catch (Exception ex)
        {
            exception = ex;
        }

        // Act
        var stackFrames = StackTraceHelper.GetFrames(exception, out _);

        // Assert
        Assert.Collection(stackFrames,
            frame =>
            {
Repository: dotnet/aspnetcore
File: src/Shared/test/Shared.Tests/StackTraceHelperTest.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;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Extensions.StackTrace.Sources;
using ThrowingLibrary;
using Xunit;

namespace Microsoft.Extensions.Internal;

public class StackTraceHelperTest
{

    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
    [Fact]
    public void StackTraceHelper_IncludesLineNumbersForFiles()
    {
        // Arrange
        Exception exception = null;
        try
        {
            // Throwing an exception in the current assembly always seems to populate the full stack
            // trace regardless of symbol type. Crossing assembly boundaries ensures PortablePdbReader gets used
            // on desktop.
            Thrower.Throw();
        }
        catch (Exception ex)
        {
            exception = ex;
        }

        // Act
        var stackFrames = StackTraceHelper.GetFrames(exception, out _);

        // Assert
        Assert.Collection(stackFrames,
            frame =>
            {
                Assert.Contains("Thrower.cs", frame.FilePath);

Review comment (suggestion):
```suggestion
    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
    
```

Doh

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;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Extensions.StackTrace.Sources;
using ThrowingLibrary;
using Xunit;

namespace Microsoft.Extensions.Internal;

public class StackTraceHelperTest
{
    private static bool IsMono => Type.GetType("Mono.RuntimeStructs") != null;
    
    [Fact]
    public void StackTraceHelper_IncludesLineNumbersForFiles()
    {
        // Arrange
        Exception exception = null;
        try
        {
            // Throwing an exception in the current assembly always seems to populate the full stack
            // trace regardless of symbol type. Crossing assembly boundaries ensures PortablePdbReader gets used
            // on desktop.
            Thrower.Throw();
        }
        catch (Exception ex)
        {
            exception = ex;
        }

        // Act
        var stackFrames = StackTraceHelper.GetFrames(exception, out _);

        // Assert
        Assert.Collection(stackFrames,
            frame =>
            {
                Assert.Contains("Thrower.cs", frame.FilePath);
Repository: dotnet/aspnetcore
File: src/Middleware/OutputCaching/src/IOutputCachePolicyProvider.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.AspNetCore.OutputCaching;

/// <summary>
/// A provider to get output cache policies for a given request.
/// </summary>
public interface IOutputCachePolicyProvider
{
    /// <summary>
    /// Gets the base output cache policies.
    /// </summary>
    /// <returns>A readonly list of <see cref="IOutputCachePolicy"/> instances.</returns>
    IReadOnlyList<IOutputCachePolicy> GetBasePolicies();

    /// <summary>
    /// Gets a <see cref="IOutputCachePolicy"/> from the given <paramref name="policyName"/>.
    /// </summary>
    /// <param name="policyName">The policy name to retrieve.</param>
    /// <returns>The named <see cref="IOutputCachePolicy"/>.</returns>
    ValueTask<IOutputCachePolicy?> GetPolicyAsync(string policyName);
}


Review comment (performance):
Doesn't it feel weird to say "for a given request" while the  provider is registered as singleton? The notion of request only appears in the NamedPolicy (OutputCacheContext).

Related question, will this be invoked for each request or only once for the application, in which case maybe registering it as transient would make sense (use and forget).

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.AspNetCore.OutputCaching;

/// <summary>
/// A provider to get output cache policies for a particular name.
/// </summary>
public interface IOutputCachePolicyProvider
{
    /// <summary>
    /// Gets the base output cache policies.
    /// </summary>
    /// <returns>A readonly list of <see cref="IOutputCachePolicy"/> instances.</returns>
    IReadOnlyList<IOutputCachePolicy> GetBasePolicies();

    /// <summary>
    /// Gets a <see cref="IOutputCachePolicy"/> from the given <paramref name="policyName"/>.
    /// </summary>
    /// <param name="policyName">The policy name to retrieve.</param>
    /// <returns>The named <see cref="IOutputCachePolicy"/>.</returns>
    ValueTask<IOutputCachePolicy?> GetPolicyAsync(string policyName);
}