ronantakizawa/codereview-bench huggingface dataset:
Repository: NativeScript/NativeScript
File: packages/webpack5/src/loaders/nativescript-hot-loader/hmr.runtime.ts (TypeScript)

Code before:

	const applyOptions = {
		ignoreUnaccepted: false,
		ignoreDeclined: false,
		ignoreErrored: false,
		onDeclined(info) {
			setStatus(hash, 'failure', 'A module has been declined.', info);
		},
		onUnaccepted(info) {
			setStatus(hash, 'failure', 'A module has not been accepted.', info);
		},
		onAccepted(info) {
			// console.log('accepted', info)
			logVerbose('Module Accepted', info);
		},
		onDisposed(info) {
			// console.log('disposed', info)
			logVerbose('Module Disposed', info);
		},
		onErrored(info) {
			setStatus(hash, 'failure', 'A module has errored.', info);
		},
	};

	const checkAndApply = async function () {
		hash = __webpack_require__.h();
		const modules = await module.hot.check().catch((error) => {
			return setStatus(
				hash,
				'failure',
				'Failed to check.',
				error.message || error.stack,
			);
		});

		if (!modules) {
			logVerbose('No modules to apply.');
			return false;
		}

		const appliedModules = await module.hot
			.apply(applyOptions)
			.catch((error) => {
				return setStatus(
					hash,
					'failure',
					'Failed to apply.',
					error.message || error.stack,
				);
			});


Review comment (bug):
I'm worrying this might be broken by someone during a future refactor due to the habit of using arrow functions in general.
We can add a comment and explain that Hermes does not support arrow functions yet.

Code after:

	const applyOptions = {
		ignoreUnaccepted: false,
		ignoreDeclined: false,
		ignoreErrored: false,
		onDeclined(info) {
			setStatus(hash, 'failure', 'A module has been declined.', info);
		},
		onUnaccepted(info) {
			setStatus(hash, 'failure', 'A module has not been accepted.', info);
		},
		onAccepted(info) {
			// console.log('accepted', info)
			logVerbose('Module Accepted', info);
		},
		onDisposed(info) {
			// console.log('disposed', info)
			logVerbose('Module Disposed', info);
		},
		onErrored(info) {
			setStatus(hash, 'failure', 'A module has errored.', info);
		},
	};

	// Important: Keep as function and not fat arrow; at the moment hermes does not support them
	const checkAndApply = async function () {
		hash = __webpack_require__.h();
		const modules = await module.hot.check().catch((error) => {
			return setStatus(
				hash,
				'failure',
				'Failed to check.',
				error.message || error.stack,
			);
		});

		if (!modules) {
			logVerbose('No modules to apply.');
			return false;
		}

		const appliedModules = await module.hot
			.apply(applyOptions)
			.catch((error) => {
				return setStatus(
					hash,
					'failure',
					'Failed to apply.',
					error.message || error.stack,
				);
			});
Repository: Netflix/consoleme
File: consoleme/handlers/v2/roles.py (Python)

Code before:
import ujson as json

from consoleme.config import config
from consoleme.handlers.base import BaseJSONHandler, BaseAPIV2Handler
from consoleme.lib.crypto import Crypto
from consoleme.lib.plugins import get_plugin_by_name

stats = get_plugin_by_name(config.get("plugins.metrics"))()
log = config.get_logger()
crypto = Crypto()
auth = get_plugin_by_name(config.get("plugins.auth"))()


class RolesHandler(BaseAPIV2Handler):
    """Handler for /api/v2/roles

    Allows read access to a list of roles across all accounts. Returned roles are
    limited to what the requesting user has access to.
    """

    allowed_methods = ["GET"]

    def initialize(self):
        self.user: str = None
        self.eligible_roles: list = []

    async def get(self):
        payload = {
            "eligible_roles": self.eligible_roles,

Review comment (suggestion):
Looks like `BaseJSONHandler` can be cleaned up from most of these files

Code after:
import ujson as json

from consoleme.config import config
from consoleme.handlers.base import BaseAPIV2Handler
from consoleme.lib.crypto import Crypto
from consoleme.lib.plugins import get_plugin_by_name

stats = get_plugin_by_name(config.get("plugins.metrics"))()
log = config.get_logger()
crypto = Crypto()
auth = get_plugin_by_name(config.get("plugins.auth"))()


class RolesHandler(BaseAPIV2Handler):
    """Handler for /api/v2/roles

    Allows read access to a list of roles across all accounts. Returned roles are
    limited to what the requesting user has access to.
    """

    allowed_methods = ["GET"]

    def initialize(self):
        self.user: str = None
        self.eligible_roles: list = []

    async def get(self):
        payload = {
            "eligible_roles": self.eligible_roles,
Repository: Netflix/consoleme
File: consoleme/handlers/v2/roles.py (Python)

Code before:
        except Exception as e:
            log_data["message"] = "Exception deleting role"
            log.error(log_data, exc_info=True)
            stats.count(
                f"{log_data['function']}.exception",
                tags={
                    "user": self.user,
                    "account": account_id,
                    "role": role_name,
                    "authorized": True,
                    "ip": self.ip,
                },
            )
            self.write_error(500, message="Error occurred deleting role: " + str(e))
            return

        # if here, role has been successfully deleted
        arn = f"arn:aws:iam::{account_id}:role/{role_name}"
        await aws.fetch_iam_role(account_id, arn, force_refresh=True)
        response_json = {
            "status": "success",
            "message": "Successfully deleted role from account",
            "role": role_name,
            "account": account_id,
        }
        self.write(json.dumps(response_json))


Review comment (suggestion):
You should be able to just `self.write(response_json)` here I believe. This would cause Tornado to automagically write the appropriate application/json response header, but I believe you're also already doing that in BaseAPIV2Handler so not a biggie

Code after:
            self.write_error(403, message="User is unauthorized to delete a role")
            return
        try:
            await delete_iam_role(account_id, role_name, self.user)
        except Exception as e:
            log_data["message"] = "Exception deleting role"
            log.error(log_data, exc_info=True)
            stats.count(
                f"{log_data['function']}.exception",
                tags={
                    "user": self.user,
                    "account": account_id,
                    "role": role_name,
                    "authorized": can_delete_role,
                    "ip": self.ip,
                },
            )
            self.write_error(500, message="Error occurred deleting role: " + str(e))
            return

        # if here, role has been successfully deleted
        arn = f"arn:aws:iam::{account_id}:role/{role_name}"
        await aws.fetch_iam_role(account_id, arn, force_refresh=True)
        response_json = {
            "status": "success",
            "message": "Successfully deleted role from account",
            "role": role_name,
            "account": account_id,
        }
        self.write(response_json)
Repository: Netflix/consoleme
File: consoleme/handlers/v2/self_service.py (Python)

Code before:
from consoleme.config import config
from consoleme.handlers.base import BaseAPIV2Handler
from consoleme.lib.policies import can_manage_policy_requests


class SelfServiceConfigHandler(BaseAPIV2Handler):
    allowed_methods = ["GET"]

    async def get(self):
        self_service_iam_config: dict = config.get("self_service_iam")
        self_service_iam_config[
            "admin_bypass_approval_enabled"
        ] = await can_manage_policy_requests(self.groups)
        self.write(self_service_iam_config)


Review comment (security):
This will introduce a security vulnerability since dictionaries are treated as references in Python So this line is actually mutating the global configuration. What we want here is something like :

```
admin_bypass_approval_enabled: bool = await can_manage_policy_requests(self.groups)
self.write({"admin_bypass_approval_enabled": admin_bypass_approval_enabled, **self_service_iam_config})
```

Code after:
from consoleme.config import config
from consoleme.handlers.base import BaseAPIV2Handler
from consoleme.lib.policies import can_manage_policy_requests


class SelfServiceConfigHandler(BaseAPIV2Handler):
    allowed_methods = ["GET"]

    async def get(self):
        admin_bypass_approval_enabled: bool = await can_manage_policy_requests(
            self.groups
        )
        self_service_iam_config: dict = config.get("self_service_iam")
        self.write(
            {
                "admin_bypass_approval_enabled": admin_bypass_approval_enabled,
                **self_service_iam_config,
            }
        )
Repository: Netflix/consoleme
File: ui/src/components/blocks/ConsoleMeDataTable.js (JavaScript)

Code before:
    const parsedQueryString = qs.parse(window.location.search, {
      ignoreQueryPrefix: true,
    });
    const filters = {};
    if (parsedQueryString) {
      Object.keys(parsedQueryString).forEach((key) => {
        filters[key] = parsedQueryString[key];
      });
    }

    this.setState({
      filters,
    });

    if (tableConfig.serverSideFiltering && Object.keys(filters).length > 0) {
      await this.filterColumnServerSide({}, filters);
    } else {
      this.filterColumnClientSide({}, filters);
    }
  }

  async filterDateRangeTime(event, data) {
    // Convert epoch milliseconds to epoch seconds
    if (data.value) {
      let startTime = 0;
      let endTime = 999999999999999;
      if (data.value.length > 0) {
        startTime = parseInt(data.value[0].getTime() / 1000, 10);
      }
      if (data.value.length > 1) {
        endTime = parseInt(data.value[1].getTime() / 1000, 10);
      }
      await this.filterColumn(_, {
        name: data.name,
        value: [startTime, endTime],
      });
    } else {
      await this.filterColumn(_, {
        name: data.name,
        value: [],
      });
    }
  }

  async filterColumn(event, data) {
    this.setState({ loading: true });
    const { name, value } = data;
    const { tableConfig } = this.state;

    const { filters } = this.state;
    filters[name] = value;

Review comment (bug):
I usually say let's use Infinity here to avoid overflow situation but you are sending this endTime to the backend. could you try Infinity or Number.MAX_SAFE_INTEGER to see what data will passed to the backend and also avoid some situation?

Code after:
    const parsedQueryString = qs.parse(window.location.search, {
      ignoreQueryPrefix: true,
    });
    const filters = {};
    if (parsedQueryString) {
      Object.keys(parsedQueryString).forEach((key) => {
        filters[key] = parsedQueryString[key];
      });
    }

    this.setState({
      filters,
    });

    if (tableConfig.serverSideFiltering && Object.keys(filters).length > 0) {
      await this.filterColumnServerSide({}, filters);
    } else {
      this.filterColumnClientSide({}, filters);
    }
  }

  async filterDateRangeTime(event, data) {
    // Convert epoch milliseconds to epoch seconds
    if (data.value) {
      let startTime = 0;
      let endTime = Number.MAX_SAFE_INTEGER;
      if (data.value.length > 0) {
        startTime = parseInt(data.value[0].getTime() / 1000, 10);
      }
      if (data.value.length > 1) {
        endTime = parseInt(data.value[1].getTime() / 1000, 10);
      }
      await this.filterColumn(_, {
        name: data.name,
        value: [startTime, endTime],
      });
    } else {
      await this.filterColumn(_, {
        name: data.name,
        value: [],
      });
    }
  }

  async filterColumn(event, data) {
    this.setState({ loading: true });
    const { name, value } = data;
    const { tableConfig } = this.state;

    const { filters } = this.state;
    filters[name] = value;
Repository: Netflix/consoleme
File: consoleme/lib/aws.py (Python)

Code before:
        )
    except Exception:
        sentry_sdk.capture_exception()
        return None
    return response["EvaluationResults"]


async def get_iam_principal_owner(arn: str, aws: Any) -> Optional[str]:
    principal_details = {}
    principal_type = arn.split(":")[-1].split("/")[0]
    account_id = arn.split(":")[4]
    # trying to find principal for subsequent queries
    if principal_type == "role":
        principal_details = await aws().fetch_iam_role(account_id, arn)
    elif principal_type == "user":
        principal_details = await aws().fetch_iam_user(account_id, arn)
    return principal_details.get("owner")


def sanitize_session_name(unsanitized_session_name):
    """
    This function sanitizes the session name typically passed in an assume_role call, to verify that it's
    """
    import re

    valid_characters_re = r"[\w+=,.@-]"

    sanitized_session_name = ""
    max_length = 64  # Session names have a length limit of 64 characters
    for char in unsanitized_session_name:
        if len(sanitized_session_name) == max_length:
            break
        if re.match(valid_characters_re, char):
            sanitized_session_name += char
    return sanitized_session_name


Review comment (performance):
To tighten this up a bit, this could be `valid_characters_re = re.compile(r"[\w+=,.@-]")`. Then line 2059 below would change to `if valid_characters_re.match(char):`.

This could be optimized further, but it doesn't seem worth it apart from this low hanging fruit change.

Code after:
        )
    except Exception:
        sentry_sdk.capture_exception()
        return None
    return response["EvaluationResults"]


async def get_iam_principal_owner(arn: str, aws: Any) -> Optional[str]:
    principal_details = {}
    principal_type = arn.split(":")[-1].split("/")[0]
    account_id = arn.split(":")[4]
    # trying to find principal for subsequent queries
    if principal_type == "role":
        principal_details = await aws().fetch_iam_role(account_id, arn)
    elif principal_type == "user":
        principal_details = await aws().fetch_iam_user(account_id, arn)
    return principal_details.get("owner")


def sanitize_session_name(unsanitized_session_name):
    """
    This function sanitizes the session name typically passed in an assume_role call, to verify that it's
    """
    valid_characters_re = re.compile(r"[\w+=,.@-]")

    sanitized_session_name = ""
    max_length = 64  # Session names have a length limit of 64 characters
    for char in unsanitized_session_name:
        if len(sanitized_session_name) == max_length:
            break
        if valid_characters_re.match(char):
            sanitized_session_name += char
    return sanitized_session_name
Repository: PaperMC/Paper
File: paper-api/src/main/java/io/papermc/paper/registry/data/DamageTypeRegistryEntry.java (Java)

Code before:
     * <strong>Note</strong> The translation key is only used if
     * {@link #deathMessageType()} is {@link DeathMessageType#DEFAULT}
     *
     * @return part of the translation key
     */
    String messageId();

    /**
     * Provides the amount of hunger exhaustion caused by this damage type.
     *
     * @return the exhaustion
     */
    Float exhaustion();

    /**
     * Provides the {@link DamageScaling} for this damage type.
     *
     * @return the damage scaling
     */
    DamageScaling damageScaling();

    /**
     * Provides the {@link DamageEffect} for this damage type.
     *
     * @return the damage effect
     */
    @Nullable
    DamageEffect damageEffect();

    /**
     * Provides the {@link DeathMessageType} for this damage type.
     *
     * @return the death message type
     */
    @Nullable
    DeathMessageType deathMessageType();

    @ApiStatus.Experimental
    @ApiStatus.NonExtendable
    interface Builder extends DamageTypeRegistryEntry, RegistryBuilder<DamageType> {
        /**
         * Sets part of the death message translation key.
         *
         * @return this builder instance.
         * @see DamageTypeRegistryEntry#messageId()
         * @see DamageType#getTranslationKey()
         */
        @Contract(value = "_ -> this", mutates = "this")
        Builder messageId(String messageId);

        /**

Review comment (suggestion):
Nullable is annotating the type, not the method so it should be on the same line (`@Nullable DamageEffect damageEffect()`)

Code after:
     * {@link #deathMessageType()} is {@link DeathMessageType#DEFAULT}
     *
     * @return part of the translation key
     */
    String messageId();

    /**
     * Provides the amount of hunger exhaustion caused by this damage type.
     *
     * @return the exhaustion
     */
    float exhaustion();

    /**
     * Provides the {@link DamageScaling} for this damage type.
     *
     * @return the damage scaling
     */
    DamageScaling damageScaling();

    /**
     * Provides the {@link DamageEffect} for this damage type.
     *
     * @return the damage effect
     */
    DamageEffect damageEffect();

    /**
     * Provides the {@link DeathMessageType} for this damage type.
     *
     * @return the death message type
     */
    DeathMessageType deathMessageType();

    /**
     * A mutable builder for the {@link DamageTypeRegistryEntry} plugins may change in applicable registry events.
     * <p>
     * The following values are required for each builder:
     * <ul>
     *     <li>{@link #messageId(String)}</li>
     *     <li>{@link #exhaustion(float)}</li>
     *     <li>{@link #damageScaling(DamageScaling)}</li>
     * </ul>
     */
    @ApiStatus.Experimental
    @ApiStatus.NonExtendable
    interface Builder extends DamageTypeRegistryEntry, RegistryBuilder<DamageType> {

        /**
         * Sets part of the death message translation key.
         *
Repository: PaperMC/Paper
File: paper-api/src/main/java/org/bukkit/event/player/PlayerRespawnEvent.java (Java)

Code before:
        Preconditions.checkArgument(respawnLocation.getWorld() != null, "Respawn world can not be null");

        this.respawnLocation = respawnLocation.clone();
    }

    @NotNull
    @Override
    public HandlerList getHandlers() {
        return handlers;
    }

    @NotNull
    public static HandlerList getHandlerList() {
        return handlers;
    }

    /**
     * An enum to specify the reason a respawn event was called.
     */
    public enum RespawnReason {

        /**
         * When the player dies and presses the respawn button.
         */
        DEATH,
        /**
         * When the player exits the end through the end portal.
         */
        END_PORTAL,
        /**
         * When a plugin respawns the player.
         */
        PLUGIN
    }

    /**
     * @deprecated in favor of {@link RespawnReason} or
     * {@link #isBedSpawn()}/{@link #isAnchorSpawn()}
     */
    @Deprecated
    public enum RespawnFlag {
        /**
         * Will use the bed spawn location
         */
        BED_SPAWN,
        /**
         * Will use the respawn anchor location
         */
        ANCHOR_SPAWN,
        /**
         * Is caused by going to the end portal in the end.

Review comment (refactor):
PlayerRespawnEvent supports mutability, this change removes that behavior as AbstractRespawnEvent clones it.

Code after:
     *
     * @param respawnLocation new location for the respawn
     */
    public void setRespawnLocation(@NotNull Location respawnLocation) {
        Preconditions.checkArgument(respawnLocation != null, "Respawn location can not be null");
        Preconditions.checkArgument(respawnLocation.getWorld() != null, "Respawn world can not be null");

        this.respawnLocation = respawnLocation.clone();
    }

    @NotNull
    @Override
    public HandlerList getHandlers() {
        return HANDLER_LIST;
    }

    @NotNull
    public static HandlerList getHandlerList() {
        return HANDLER_LIST;
    }

    /**
     * An enum to specify the reason a respawn event was called.
     */
    public enum RespawnReason {

        /**
         * When the player dies and presses the respawn button.
         */
        DEATH,
        /**
         * When the player exits the end through the end portal.
         */
        END_PORTAL,
        /**
         * When a plugin respawns the player.
         */
        PLUGIN
    }

    public enum RespawnFlag {
        /**
         * Will use the bed spawn location
         */
        BED_SPAWN,
        /**
         * Will use the respawn anchor location
         */
        ANCHOR_SPAWN,
        /**
         * Is caused by going to the end portal in the end.
Repository: PaperMC/Paper
File: paper-server/src/main/java/io/papermc/paper/command/brigadier/PaperCommandSourceStack.java (Java)

Code before:
package io.papermc.paper.command.brigadier;

import com.destroystokyo.paper.brigadier.BukkitBrigadierCommandSource;
import net.minecraft.world.level.Level;
import net.minecraft.world.phys.Vec2;
import net.minecraft.world.phys.Vec3;
import org.bukkit.Location;
import org.bukkit.command.CommandSender;
import org.bukkit.craftbukkit.entity.CraftEntity;
import org.bukkit.entity.Entity;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.jspecify.annotations.NonNull;

public interface PaperCommandSourceStack extends CommandSourceStack, BukkitBrigadierCommandSource {

    net.minecraft.commands.CommandSourceStack getHandle();

    @Override
    default @NotNull Location getLocation() {
        Vec2 rot = this.getHandle().getRotation();
        Vec3 pos = this.getHandle().getPosition();
        Level level = this.getHandle().getLevel();

        return new Location(level.getWorld(), pos.x, pos.y, pos.z, rot.y, rot.x);
    }

    @Override
    @NotNull
    default CommandSender getSender() {
        return this.getHandle().getBukkitSender();
    }

    @Override
    @Nullable
    default Entity getExecutor() {
        net.minecraft.world.entity.Entity nmsEntity = this.getHandle().getEntity();

Review comment (suggestion):
Don't mix nullability annotations either convert the class to jspecify or keep using jetbrains one for now

Code after:
package io.papermc.paper.command.brigadier;

import com.destroystokyo.paper.brigadier.BukkitBrigadierCommandSource;
import com.google.common.base.Preconditions;
import net.minecraft.world.level.Level;
import net.minecraft.world.phys.Vec2;
import net.minecraft.world.phys.Vec3;
import org.bukkit.Location;
import org.bukkit.command.CommandSender;
import org.bukkit.craftbukkit.entity.CraftEntity;
import org.bukkit.entity.Entity;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;

public interface PaperCommandSourceStack extends CommandSourceStack, BukkitBrigadierCommandSource {

    net.minecraft.commands.CommandSourceStack getHandle();

    @Override
    default @NonNull Location getLocation() {
        Vec2 rot = this.getHandle().getRotation();
        Vec3 pos = this.getHandle().getPosition();
        Level level = this.getHandle().getLevel();

        return new Location(level.getWorld(), pos.x, pos.y, pos.z, rot.y, rot.x);
    }

    @Override
    @NonNull
    default CommandSender getSender() {
        return this.getHandle().getBukkitSender();
    }

    @Override
    @Nullable
    default Entity getExecutor() {
        net.minecraft.world.entity.Entity nmsEntity = this.getHandle().getEntity();