ronantakizawa/codereview-bench huggingface dataset:
Repository: micronaut-projects/micronaut-core
File: inject/src/main/java/io/micronaut/context/event/BeanCreatedEvent.java (Java)

Code before:
     * @param bean           The bean
     */
    public BeanCreatedEvent(BeanContext beanContext,
                            BeanDefinition<T> beanDefinition,
                            BeanIdentifier beanIdentifier,
                            Argument<T> beanType,
                            T bean) {
        super(beanContext, beanDefinition, bean);
        this.beanIdentifier = beanIdentifier;
        this.beanType = beanType;
    }

    /**
     * @return The bean identifier used to create the bean
     */
    public BeanIdentifier getBeanIdentifier() {
        return beanIdentifier;
    }

    /**
     * @return The argument used to create the bean
     */
    public Argument<T> getBeanType() {
        return beanType;
    }
}


Review comment (suggestion):
please add `@since 4.9.0` and `@io.micronaut.core.annotation.NonNull` to the return type

Code after:
     * @param beanType       The {@link Argument} used to create the bean
     * @param bean           The bean
     */
    public BeanCreatedEvent(BeanContext beanContext,
                            BeanDefinition<T> beanDefinition,
                            BeanIdentifier beanIdentifier,
                            @NonNull Argument<T> beanType,
                            T bean) {
        super(beanContext, beanDefinition, bean);
        this.beanIdentifier = beanIdentifier;
        this.beanType = beanType;
    }

    /**
     * @return The bean identifier used to create the bean
     */
    public BeanIdentifier getBeanIdentifier() {
        return beanIdentifier;
    }

    /**
     * @return The argument used to create the bean
     * @since 4.9.0
     */
    @NonNull
    public Argument<T> getBeanType() {
        return beanType;
    }
}
Repository: micronaut-projects/micronaut-core
File: retry/src/main/java/io/micronaut/retry/annotation/Retryable.java (Java)

Code before:
     * @return The maximum overall delay
     */
    String maxDelay() default "";

    /**
     * @return The multiplier to use to calculate the delay
     */
    @Digits(integer = 2, fraction = 2)
    String multiplier() default "1.0";

    /**
     * @return The retry predicate class to use instead of {@link Retryable#includes} and {@link Retryable#excludes}
     * (defaults to none)
     */
    Class<? extends RetryPredicate> predicate() default DefaultRetryPredicate.class;

    /**
     * @return The capture exception types (defaults to RuntimeException)
     */
    Class<? extends Throwable> capturedException() default RuntimeException.class;

    /**
     * @return The jitter factor used to apply random deviation to retry delays
     */
    @Digits(integer = 1, fraction = 2)
    String jitter() default "0.25";
}


Review comment (suggestion):
```suggestion
    String jitter() default "0.0";
```
should be set default jitter to `0`? (jitter `0` means don't apply jitter by default)

Code after:
     * @return The maximum overall delay
     */
    String maxDelay() default "";

    /**
     * @return The multiplier to use to calculate the delay
     */
    @Digits(integer = 2, fraction = 2)
    String multiplier() default "1.0";

    /**
     * @return The retry predicate class to use instead of {@link Retryable#includes} and {@link Retryable#excludes}
     * (defaults to none)
     */
    Class<? extends RetryPredicate> predicate() default DefaultRetryPredicate.class;

    /**
     * @return The capture exception types (defaults to RuntimeException)
     */
    Class<? extends Throwable> capturedException() default RuntimeException.class;

    /**
     * @return The jitter factor used to apply random deviation to retry delays
     */
    @Digits(integer = 1, fraction = 2)
    String jitter() default "0.0";
}
Repository: micronaut-projects/micronaut-core
File: retry/src/main/java/io/micronaut/retry/intercept/SimpleRetry.java (Java)

Code before:
        return predicate;
    }

    @Override
    public Class<? extends Throwable> getCapturedException() {
        return capturedException;
    }

    /**
     * @return The jitter factor used to apply random deviation to retry delays
     */
    @Override
    public OptionalDouble getJitter() {
        return jitter > 0 ? OptionalDouble.of(jitter) : OptionalDouble.empty();
    }

    /**
     * @return Return the millisecond value for the next delay
     */
    @Override
    @Internal
    public long nextDelay() {
        double multiplier = getMultiplier().orElse(1.0);
        long delay = (long) ((getDelay().toMillis()) * pow(multiplier, attemptNumber.get() - 1));
        double jitter = getJitter().orElse(0);
        if(jitter > 0) {
            delay = Math.max(0, (long) (delay * (1.0 + ThreadLocalRandom.current().nextDouble(-jitter, jitter))));
        }
        overallDelay.addAndGet(delay);
        return delay;
    }
}


Review comment (suggestion):
```suggestion
        if (jitter > 0) {
```
nit. indent

Code after:
        return predicate;
    }

    @Override
    public Class<? extends Throwable> getCapturedException() {
        return capturedException;
    }

    /**
     * @return The jitter factor used to apply random deviation to retry delays
     */
    @Override
    public OptionalDouble getJitter() {
        return jitter > 0 ? OptionalDouble.of(jitter) : OptionalDouble.empty();
    }

    /**
     * @return Return the millisecond value for the next delay
     */
    @Override
    @Internal
    public long nextDelay() {
        double multiplier = getMultiplier().orElse(1.0);
        long delay = (long) ((getDelay().toMillis()) * pow(multiplier, attemptNumber.get() - 1));
        double jitter = getJitter().orElse(0);
        if (jitter > 0) {
            delay = Math.max(0, (long) (delay * (1.0 + ThreadLocalRandom.current().nextDouble(-jitter, jitter))));
        }
        overallDelay.addAndGet(delay);
        return delay;
    }
}
Repository: micronaut-projects/micronaut-core
File: http/src/main/java/io/micronaut/http/resource/ResourceLoaderFactory.java (Java)

Code before:
    /**
     * @return The class path resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull ClassPathResourceLoader getClassPathResourceLoader() {
        return new DefaultClassPathResourceLoader(classLoader);
    }

    /**
     * @return The file system resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull FileSystemResourceLoader fileSystemResourceLoader() {
        return new DefaultFileSystemResourceLoader();
    }

    /**
     * @param resourceLoaders The resource loaders
     * @return The resource resolver
     */
    @Singleton
    @BootstrapContextCompatible
    @Indexed(ResourceResolver.class)
    protected @NonNull ResourceResolver resourceResolver(@NonNull List<ResourceLoader> resourceLoaders) {
        return new ResourceResolver(resourceLoaders);
    }
}


Review comment (suggestion):
I wonder if we can remove those as beans, and simply have some collection filled without the bean context

Code after:
    }

    /**
     * @return The class path resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull ClassPathResourceLoader getClassPathResourceLoader() {
        return new DefaultClassPathResourceLoader(classLoader);
    }

    /**
     * @return The file system resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull FileSystemResourceLoader fileSystemResourceLoader() {
        return new DefaultFileSystemResourceLoader();
    }

    /**
     * @return The string resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull ResourceLoader getStringResourceLoader() {
        return StringResourceLoader.getInstance();
    }

    /**
     * @return The base64 resource loader
     */
    @Singleton
    @BootstrapContextCompatible
    protected @NonNull ResourceLoader getBase64ResourceLoader() {
        return Base64ResourceLoader.getInstance();
    }

    /**
     * @param resourceLoaders The resource loaders
     * @return The resource resolver
     */
    @Singleton
    @BootstrapContextCompatible
    @Indexed(ResourceResolver.class)
    protected @NonNull ResourceResolver resourceResolver(@NonNull List<ResourceLoader> resourceLoaders) {
        return new ResourceResolver(resourceLoaders);
    }
}
Repository: micronaut-projects/micronaut-core
File: http/src/main/java/io/micronaut/http/bind/binders/DefaultBodyAnnotationBinder.java (Java)

Code before:
    /**
     * Try to bind from the <i>full</i> body of the request to a {@link ConvertibleValues} for
     * argument spreading.
     *
     * @param source The request
     * @return The body as a {@link ConvertibleValues} instance
     */
    @SuppressWarnings({"unchecked", "rawtypes"})
    protected BindingResult<ConvertibleValues<?>> bindFullBodyConvertibleValues(HttpRequest<?> source) {
        Optional<ConvertibleValues> convertibleValuesBody = source.getBody(ConvertibleValues.class);
        return () -> (Optional) convertibleValuesBody;
    }

    /**
     * Try to bind from the <i>full</i> body of the request, i.e. no argument spreading.
     *
     * @param context The conversion context
     * @param source  The request
     * @return The binding result
     */
    public BindingResult<T> bindFullBody(ArgumentConversionContext<T> context, HttpRequest<?> source) {
        Optional<?> body = source.getBody();
        if (body.isPresent()) {
            return doConvert(body.get(), context);
        }
        Optional<String> defaultValue = context.getAnnotationMetadata().stringValue(Body.class, "defaultValue");
        return defaultValue.isPresent() ? doConvert(defaultValue.get(), context) : BindingResult.empty();
    }
}


Review comment (refactor):
same here

maybe you could extract this into a `bindDefaultValue` method that you could call directly from the netty impl, instead of having to duplicate this code

Code after:

    /**
     * Try to bind from the <i>full</i> body of the request to a {@link ConvertibleValues} for
     * argument spreading.
     *
     * @param source The request
     * @return The body as a {@link ConvertibleValues} instance
     */
    @SuppressWarnings({"unchecked", "rawtypes"})
    protected BindingResult<ConvertibleValues<?>> bindFullBodyConvertibleValues(HttpRequest<?> source) {
        Optional<ConvertibleValues> convertibleValuesBody = source.getBody(ConvertibleValues.class);
        return () -> (Optional) convertibleValuesBody;
    }

    /**
     * Try to bind from the <i>full</i> body of the request, i.e. no argument spreading.
     *
     * @param context The conversion context
     * @param source  The request
     * @return The binding result
     */
    public BindingResult<T> bindFullBody(ArgumentConversionContext<T> context, HttpRequest<?> source) {
        Optional<?> body = source.getBody();
        if (body.isPresent()) {
            return doConvert(body.get(), context);
        }
        return bindDefaultValue(context);
    }

    /**
     * Try to bind the <i>defaultValue</i>, if any specified.
     *
     * @param context The conversion context
     * @return The binding result
     */
    protected BindingResult<T> bindDefaultValue(ArgumentConversionContext<T> context) {
        Optional<String> defaultValue = context.getAnnotationMetadata().stringValue(Bindable.class, "defaultValue");
        return defaultValue.isPresent() ? doConvert(defaultValue.get(), context) : BindingResult.empty();
    }
}
Repository: microsoft/ApplicationInspector
File: AppInspector.RulesEngine/Ruleset.cs (C#)

Code before:
﻿// Copyright (C) Microsoft. All rights reserved. Licensed under the MIT License.


using Microsoft.Extensions.Logging;

namespace Microsoft.ApplicationInspector.RulesEngine;

/// <summary>
///     Default class to use to store Application Inspector <see cref="Rule" /> objects.
/// </summary>
public class RuleSet : TypedRuleSet<Rule>
{
    /// <summary>
    ///     Create a ruleset using the given (optional) logger.
    /// </summary>
    /// <param name="loggerFactory"></param>
    public RuleSet(ILoggerFactory? loggerFactory = null, bool enableNonBacktrackingRegex = false) 
        : base(loggerFactory)
    {
        EnableNonBacktrackingRegex = enableNonBacktrackingRegex;
    }
}

Review comment (suggestion):
Nit: If this is specified in the constructor, I think it would make more sense to pass down to the `TypedRuleSet` constructor to set the parameter at that level. DevSkim 0.8 uses the TypedRuleSet class because its Rule objects are slightly different so it will also need a way to set this parameter as well.

Alternately, I think it could be okay to make hte parameter public and not have it in the constructor at all and use init syntax like

`var ruleset = new RuleSet(loggerFactory){ EnableNonBackTrackingRegex = _opts.EnableNonBackTrackingRegex };`

Where it needs to be set.

This has the advantage of not changing existing public APIs.

Alternately, I think it makes sense to me to have instead a `RuleSetOptions`, which for now would only have `EnableNonBackTrackingRegex` but allows adding additional parameters to the constructor in the future wihtout changing the signature by adding properties to the options object.

Code after:
﻿// Copyright (C) Microsoft. All rights reserved. Licensed under the MIT License.


using Microsoft.Extensions.Logging;

namespace Microsoft.ApplicationInspector.RulesEngine;

/// <summary>
///     Default class to use to store Application Inspector <see cref="Rule" /> objects.
/// </summary>
public class RuleSet : TypedRuleSet<Rule>
{
    /// <summary>
    ///     Create a ruleset using the given (optional) logger.
    /// </summary>
    /// <param name="loggerFactory"></param>
    public RuleSet(ILoggerFactory? loggerFactory = null) 
        : base(loggerFactory)
    {    
    }
}
Repository: microsoft/CBL-Mariner
File: .pipelines/containerSourceData/scripts/BuildGoldenDistrolessContainer.sh (Shell)

Code before:
#!/bin/bash
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

set -e

function docker_cleanup {
    echo "+++ Cleaning up Docker resources to free disk space"
    docker system prune -f --volumes 2>/dev/null || true
    docker builder prune -f 2>/dev/null || true
    echo "+++ Docker cleanup completed"
    df -h / || true
}

function DockerBuild {
    local containerName=$1
    local azureLinuxVersion=$2
    local imageType=$3
    local packagesToInstall=$4
    local packagesToHoldback=$5
    local installNonrootUser=$6
    local rpmsDir=$7
    local user=root
    local userUid=0

    if $installNonrootUser; then
        user="nonroot"
        userUid=65532
    fi

    # Create container
    echo "+++ Create container $containerName"

    # DOCKER_BUILDKIT=0 is set to avoid the unknown timeout error in the Azure DevOps pipeline.
    # The error is likely caused by some BuildKit feature in version 24.0.9 of moby-engine.
    # The error is not seen in the local environment.
    # Setting DOCKER_BUILDKIT=0 disables BuildKit and uses the legacy builder.

Review comment (bug):
Why `|| true` and the stderr redirect? If we want to continue in case of failure, then I'd at least leave the errors for easier debugging.

Code after:
#!/bin/bash
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

set -e

function docker_cleanup {
    echo "+++ Cleaning up Docker resources to free disk space"
    docker system prune -f --volumes || true
    docker builder prune -f || true
    echo "+++ Docker cleanup completed"
    df -h / || true
}

function DockerBuild {
    local containerName=$1
    local azureLinuxVersion=$2
    local imageType=$3
    local packagesToInstall=$4
    local packagesToHoldback=$5
    local installNonrootUser=$6
    local rpmsDir=$7
    local user=root
    local userUid=0

    if $installNonrootUser; then
        user="nonroot"
        userUid=65532
    fi

    # Create container
    echo "+++ Create container $containerName"

    # DOCKER_BUILDKIT=0 is set to avoid the unknown timeout error in the Azure DevOps pipeline.
    # The error is likely caused by some BuildKit feature in version 24.0.9 of moby-engine.
    # The error is not seen in the local environment.
    # Setting DOCKER_BUILDKIT=0 disables BuildKit and uses the legacy builder.
Repository: microsoft/CBL-Mariner
File: scripts/build-vm-images.sh (Shell)

Code before:
#!/bin/bash
set -euxo pipefail
TARGET_DIR="/var/tmp/azl-vm-images"

# remove the target dir if it exists
if [ -d "$TARGET_DIR" ]; then
    sudo rm -rf "$TARGET_DIR"
fi

sudo rm -rf ./base/out/images/*
sudo rm -rf ./base/build/work/vm-base/*
# # Build the VM image using KIWI
# sudo kiwi --loglevel 10 \
#     --kiwi-file vm-base.kiwi \
#     system build  \
#     --description ./base/images/vm-base \
#     --target-dir "$TARGET_DIR" \
#     --add-repo="$REMOTE_KOJI_REPO_URL"

# Build vm-base image using azldev
azldev image build vm-base --local-repo ./base/out --remote-repo "$REMOTE_KOJI_REPO_URL"

# Convert VHDX to VPC fixed size
qemu-img convert -f vhdx -O vpc -o subformat=fixed,force_size \
    ./base/out/images/vm-base/azl4-vm-base.x86_64-0.1.vhdx \
    ./base/out/images/vm-base/azl4-vm-base.x86_64-0.1.vhd

# Copy the resulting VHD to Windows Downloads folder

Review comment (suggestion):
issue: This creates a target directory in the user's /var/tmp namespace, which is not guaranteed to be safe to manipulate.

Instead, consider reworking to utilize `mktemp -d` invocation so you get a unique temp directory. This command also has the possible benefit, depending on how the system is configured, to create the directory in a tmpfs, which is backed by RAM for faster I/O transactions than a directory backed by a storage disk media

Code after:
#!/bin/bash
set -euxo pipefail

# Find the absolute path of the directory containing this script
SCRIPTS_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
. "$SCRIPTS_DIR/common.sh"

sudo rm -rf "$REPO_ROOT/base/build/work/vm-base/*"
sudo rm -rf "$REPO_ROOT/base/out/images/*"

# Build vm-base image using azldev
azldev image build vm-base --local-repo "$REPO_ROOT/base/out" --remote-repo "$REMOTE_KOJI_REPO_URL" --remote-repo-no-gpgcheck
Repository: microsoft/CBL-Mariner
File: scripts/build-vm-images.sh (Shell)

Code before:
#!/bin/bash
set -euxo pipefail

sudo rm -rf ./base/out/images/*
sudo rm -rf ./base/build/work/vm-base/*
# Find the absolute path of the directory containing this script
SCRIPTS_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
. "$SCRIPTS_DIR/common.sh"

# Build vm-base image using azldev
azldev image build vm-base --local-repo ./base/out --remote-repo "$REMOTE_KOJI_REPO_URL"

# Convert VHDX to VPC fixed size
qemu-img convert -f vhdx -O vpc -o subformat=fixed,force_size \
    ./base/out/images/vm-base/azl4-vm-base.x86_64-0.1.vhdx \
    ./base/out/images/vm-base/azl4-vm-base.x86_64-0.1.vhd


Review comment (suggestion):
Suggestion: I have the same observation as copilot. An alternative is to find git root `git rev-parse --show-toplevel` and base paths from that. It would also help to build out common prefix paths, that is a coding pattern.

Code after:
#!/bin/bash
set -euxo pipefail

# Find the absolute path of the directory containing this script
SCRIPTS_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
. "$SCRIPTS_DIR/common.sh"

sudo rm -rf "$REPO_ROOT/base/build/work/vm-base/*"
sudo rm -rf "$REPO_ROOT/base/out/images/*"

# Build vm-base image using azldev
azldev image build vm-base --local-repo "$REPO_ROOT/base/out" --remote-repo "$REMOTE_KOJI_REPO_URL" --remote-repo-no-gpgcheck
Repository: microsoft/RD-Agent
File: rdagent/app/cli.py (Python)

Code before:
"""
CLI entrance for all rdagent application.

This will 
- make rdagent a nice entry and
- autoamtically load dotenv
"""
from dotenv import load_dotenv

load_dotenv(".env")

import subprocess
from importlib.resources import path as rpath

import fire

from rdagent.app.data_mining.model import main as med_model
from rdagent.app.general_model.general_model import (
    extract_models_and_implement as general_model,
)
from rdagent.app.qlib_rd_loop.factor import main as fin_factor
from rdagent.app.qlib_rd_loop.factor_from_report import main as fin_factor_report
from rdagent.app.qlib_rd_loop.model import main as fin_model
from rdagent.app.utils.info import collect_info


def ui(port=80, log_dir="", debug=False):
    """
    start web app to show the log traces.
    """
    with rpath("rdagent.log.ui", "app.py") as app_path:
        cmds = ["streamlit", "run", app_path, f"--server.port={port}"]
        if log_dir or debug:
            cmds.append("--")
        if log_dir:

Review comment (suggestion):
```suggestion
load_dotenv(".env")
# 1) Make sure it is at the beginning of the script so that it will load dotenv before initializing BaseSettings.
# 2) The ".env" argument is necessary to make sure it loads `.env` from the current directory.
```

Code after:
"""
CLI entrance for all rdagent application.

This will 
- make rdagent a nice entry and
- autoamtically load dotenv
"""
from dotenv import load_dotenv

load_dotenv(".env")
# 1) Make sure it is at the beginning of the script so that it will load dotenv before initializing BaseSettings.
# 2) The ".env" argument is necessary to make sure it loads `.env` from the current directory.

import subprocess
from importlib.resources import path as rpath

import fire

from rdagent.app.data_mining.model import main as med_model
from rdagent.app.general_model.general_model import (
    extract_models_and_implement as general_model,
)
from rdagent.app.qlib_rd_loop.factor import main as fin_factor
from rdagent.app.qlib_rd_loop.factor_from_report import main as fin_factor_report
from rdagent.app.qlib_rd_loop.model import main as fin_model
from rdagent.app.utils.info import collect_info


def ui(port=80, log_dir="", debug=False):
    """
    start web app to show the log traces.
    """
    with rpath("rdagent.log.ui", "app.py") as app_path:
        cmds = ["streamlit", "run", app_path, f"--server.port={port}"]
        if log_dir or debug:
Repository: microsoft/RD-Agent
File: rdagent/log/time.py (Python)

Code before:
import time
from functools import wraps


def measure_time(method):
    @wraps(method)
    def timed(*args, **kwargs):
        self = args[0]
        start_time = time.time()
        result = method(*args, **kwargs)
        end_time = time.time()
        duration = end_time - start_time
        method_name = method.__name__
        self.times[method_name] = duration
        return result

    return timed


Review comment (suggestion):
check exist of self.times here? if not exist, self.times = {}. then delete `self.times={}` in outer class (RDLoop Class)

Code after:
import time
from functools import wraps

from rdagent.log import rdagent_logger as logger


def measure_time(method):
    @wraps(method)
    def timed(*args, **kwargs):
        start_time = time.time()
        result = method(*args, **kwargs)
        end_time = time.time()
        duration = end_time - start_time
        method_name = method.__name__
        # logger.log_object(f"{method_name} took {duration:.2f} sec")
        logger.info(f"{method_name} took {duration:.2f} sec")
        return result

    return timed