ronantakizawa/codereview-bench huggingface dataset:
Repository: lm-sys/FastChat
File: fastchat/serve/api_provider.py (Python)

Code before:
            text += data
            yield {"text": text, "error_code": 0}


def cohere_api_stream_iter(
    client_name: str,
    model_id: str,
    messages: list,
    temperature: Optional[
        float
    ] = None,  # The SDK or API handles None for all parameters following
    top_p: Optional[float] = None,
    max_new_tokens: Optional[int] = None,
    api_key: Optional[str] = None,  # default is env var CO_API_KEY
    api_base: Optional[str] = None,
):
    import cohere

    OPENAI_TO_COHERE_ROLE_MAP = {
        "user": "User",
        "assistant": "Chatbot",
        "system": "System",
    }

    client = cohere.Client(
        api_key=api_key or os.getenv("CO_API_KEY"),  # fern sdk doesn't accept None
        base_url=api_base,
        client_name=client_name or "FastChat",
    )

    # prepare and log requests
    chat_history = [
        dict(
            role=OPENAI_TO_COHERE_ROLE_MAP[message["role"]], message=message["content"]
        )
        for message in messages[:-1]
    ]
    actual_prompt = messages[-1]["content"]

    gen_params = {
        "model": model_id,
        "messages": messages,
        "chat_history": chat_history,
        "prompt": actual_prompt,
        "temperature": temperature,
        "top_p": top_p,
        "max_new_tokens": max_new_tokens,
    }
    logger.info(f"==== request ====\n{gen_params}")

    # make request and stream response

Review comment (suggestion):
```suggestion
```

cohere>=5.0.1 should pick up CO_API_KEY correctly but will fix the issue with accepting None!

Code after:
            text += data
            yield {"text": text, "error_code": 0}


def cohere_api_stream_iter(
    client_name: str,
    model_id: str,
    messages: list,
    temperature: Optional[
        float
    ] = None,  # The SDK or API handles None for all parameters following
    top_p: Optional[float] = None,
    max_new_tokens: Optional[int] = None,
    api_key: Optional[str] = None,  # default is env var CO_API_KEY
    api_base: Optional[str] = None,
):
    import cohere

    OPENAI_TO_COHERE_ROLE_MAP = {
        "user": "User",
        "assistant": "Chatbot",
        "system": "System",
    }

    client = cohere.Client(
        api_key=api_key,
        base_url=api_base,
        client_name=client_name or "FastChat",
    )

    # prepare and log requests
    chat_history = [
        dict(
            role=OPENAI_TO_COHERE_ROLE_MAP[message["role"]], message=message["content"]
        )
        for message in messages[:-1]
    ]
    actual_prompt = messages[-1]["content"]

    gen_params = {
        "model": model_id,
        "messages": messages,
        "chat_history": chat_history,
        "prompt": actual_prompt,
        "temperature": temperature,
        "top_p": top_p,
        "max_new_tokens": max_new_tokens,
    }
    logger.info(f"==== request ====\n{gen_params}")

    # make request and stream response
Repository: lm-sys/FastChat
File: fastchat/serve/monitor/add_markdown_info.py (Python)

Code before:
#             print(e)
#             results.append((-1, -1, -1))

#     return results


def get_element_counts(df, column="conversation_a"):
    pattern = re.compile("```([^`]*)```")
    answers = df[column].map(
        lambda convo: "\n".join(
            [turn["content"] for turn in convo if turn["role"] == "assistant"]
        )
    )
    results = answers.progress_map(
        lambda answer: count_markdown_elements(remove_code(answer, pattern))
    )

    return results.tolist()


def add_markdown_meta(row):
    conv_meta = {k: v for k, v in row["conv_metadata"].items()}
    conv_meta["sum_bold_count_a"] = row["markdown_meta_a"][0]
    conv_meta["sum_list_count_a"] = row["markdown_meta_a"][1]
    conv_meta["sum_header_count_a"] = row["markdown_meta_a"][2]

    conv_meta["sum_bold_count_b"] = row["markdown_meta_b"][0]
    conv_meta["sum_list_count_b"] = row["markdown_meta_b"][1]
    conv_meta["sum_header_count_b"] = row["markdown_meta_b"][2]

    return conv_meta


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--input-file", type=str, required=True)
    parser.add_argument("--output-file", type=str, required=True)
    args = parser.parse_args()

    print("loading file...")
    data = pd.read_json(args.input_file)

    assert "conv_metadata" in data.columns

    temp = data[["question_id", "conv_metadata"]].copy()

    print("Processing conversation_a")
    temp["markdown_meta_a"] = get_element_counts(data, column="conversation_a")

    print("Processing conversation_b")
    temp["markdown_meta_b"] = get_element_counts(data, column="conversation_b")

Review comment (suggestion):
```suggestion
    conv_meta["markdown"]["bold_cnt"] = {"bold": 10}
    conv_meta["markdown"]["list_count_a"] = {"order": 10, "unorder": 5}
    conv_meta["markdown"]["header_count_a"] = {"h1": 5, "h2" 3, ...}
```

Code after:

    return results.tolist()


def add_markdown_meta(row):
    conv_meta = {k: v for k, v in row["conv_metadata"].items()}
    return conv_meta | row["markdown_meta_a"] | row["markdown_meta_b"]


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument("--input-file", type=str, required=True)
    parser.add_argument("--output-file", type=str, required=True)
    args = parser.parse_args()

    print("loading file...")
    data = pd.read_json(args.input_file)

    assert "conv_metadata" in data.columns

    temp = data[["question_id", "conv_metadata"]].copy()

    print("Processing conversation_a")
    temp["markdown_meta_a"] = get_element_counts(data, column="conversation_a")

    print("Processing conversation_b")
    temp["markdown_meta_b"] = get_element_counts(data, column="conversation_b")

    print("Post-processing...")
    data["conv_metadata"] = temp.apply(add_markdown_meta, axis=1)

    print("Saving to file...")
    data.to_json(args.output_file, orient="records", indent=4, force_ascii=False)
Repository: locustio/locust
File: examples/nested_inline_tasksets.py (Python)

Code before:
from locust import HttpUser, TaskSet, task, between


class WebsiteUser(HttpUser):
    """
    Example of the ability of inline nested TaskSet classes
    """
    host = "http://127.0.0.1:8089"
    wait_time = between(2, 5)
    
    class TopLevelTaskSet(TaskSet):
        @task
        class IndexTaskSet(TaskSet):
            @task(10)
            def index(self):
                self.client.get("/")
            
            @task(1)
            def stop(self):
                self.interrupt()
        
        @task
        def stats(self):
            self.client.get("/stats/requests")

    tasks = [TopLevelTaskSet]

Review comment (suggestion):
We could also decorate the `TopLevelTaskSet` with `@task` instead of setting the `tasks` attribute.

Code after:
from locust import HttpUser, TaskSet, task, between


class WebsiteUser(HttpUser):
    """
    Example of the ability of inline nested TaskSet classes
    """
    host = "http://127.0.0.1:8089"
    wait_time = between(2, 5)

    @task
    class TopLevelTaskSet(TaskSet):
        @task
        class IndexTaskSet(TaskSet):
            @task(10)
            def index(self):
                self.client.get("/")
            
            @task(1)
            def stop(self):
                self.interrupt()
        
        @task
        def stats(self):
            self.client.get("/stats/requests")
Repository: locustio/locust
File: locust/input_events.py (Python)

Code before:
import time

isWindows = False
try:
    from win32api import STD_INPUT_HANDLE
    from win32console import (
        GetStdHandle,
        KEY_EVENT,
        ENABLE_ECHO_INPUT,
        ENABLE_LINE_INPUT,
        ENABLE_PROCESSED_INPUT,
    )

    isWindows = True
except ImportError:
    import sys
    import select
    import termios
    import tty


class KeyPoller:
    def __enter__(self):
        if isWindows:
            self.read_handle = GetStdHandle(STD_INPUT_HANDLE)
            self.read_handle.SetConsoleMode(ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT | ENABLE_PROCESSED_INPUT)

Review comment (suggestion):
Use gevent.sleep instead of time.sleep. They actually mean the same thing, but only because gevent has monkey patched it, and locust seems to use gevent.sleep throughout...

Code after:
import gevent
import logging
import os

if os.name == "nt":
    from win32api import STD_INPUT_HANDLE
    from win32console import (
        GetStdHandle,
        KEY_EVENT,
        ENABLE_ECHO_INPUT,
        ENABLE_LINE_INPUT,
        ENABLE_PROCESSED_INPUT,
    )
else:
    import sys
    import select
    import termios
    import tty


class UnixKeyPoller:
    def __init__(self):
        try:
            self.stdin = sys.stdin.fileno()
            self.tattr = termios.tcgetattr(self.stdin)
            tty.setcbreak(self.stdin, termios.TCSANOW)
Repository: locustio/locust
File: locust/input_events.py (Python)

Code before:
                            pass
                        else:
                            cur_char = str(cur_event.Char)
                            self.captured_chars.append(cur_char)

                self.cur_event_length = len(events_peek)

            if not len(self.captured_chars) == 0:
                return self.captured_chars.pop(0)
            else:
                return None
        else:
            dr, dw, de = select.select([sys.stdin], [], [], 0)
            if not dr == []:
                return sys.stdin.read(1)
            return None


def input_listener(key_to_func_map):
    def input_listener_func():
        with KeyPoller() as poller:
            map = key_to_func_map
            while True:
                input = poller.poll()
                if input is not None:
                    print(f"Current input is: {input}")
                    for key in map:
                        if input == key:
                            map[key]()
                else:
                    time.sleep(0.2)

    return input_listener_func


Review comment (bug):
this isnt supposed to be here right? :) (could be useful as a logging.debug statement though)

Code after:

        if not len(events_peek) == self.cur_event_length:
            for cur_event in events_peek[self.cur_event_length :]:
                if cur_event.EventType == KEY_EVENT:
                    if ord(cur_event.Char) and cur_event.KeyDown:
                        cur_char = str(cur_event.Char)
                        self.captured_chars.append(cur_char)

            self.cur_event_length = len(events_peek)

        if self.captured_chars:
            return self.captured_chars.pop(0)
        else:
            return None


def get_poller():
    if os.name == "nt":
        return WindowsKeyPoller()
    else:
        return UnixKeyPoller()


def input_listener(key_to_func_map):
    poller = get_poller()

    def input_listener_func():
        try:
            while True:
                input = poller.poll()
                if input is not None:
                    for key in key_to_func_map:
                        if input == key:
                            key_to_func_map[key]()
                else:
                    gevent.sleep(0.2)
        except Exception as e:
            logging.warning(f"Exception in keyboard input poller: {e}")
        finally:
            poller.cleanup()

    return input_listener_func
Repository: locustio/locust
File: locust/input_events.py (Python)

Code before:
import time

isWindows = False
try:
    from win32api import STD_INPUT_HANDLE
    from win32console import (
        GetStdHandle,
        KEY_EVENT,
        ENABLE_ECHO_INPUT,
        ENABLE_LINE_INPUT,
        ENABLE_PROCESSED_INPUT,
    )

    isWindows = True
except ImportError:
    import sys
    import select
    import termios
    import tty


class KeyPoller:
    def __enter__(self):
        if isWindows:
            self.read_handle = GetStdHandle(STD_INPUT_HANDLE)
            self.read_handle.SetConsoleMode(ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT | ENABLE_PROCESSED_INPUT)

            self.cur_event_length = 0

Review comment (suggestion):
This is a nicer way to know if we're on Windows (no need to try/catch & no need to store our own variable)

```
import os

if os.name == 'nt':
     ...
```

Code after:
import gevent
import logging
import os

if os.name == "nt":
    from win32api import STD_INPUT_HANDLE
    from win32console import (
        GetStdHandle,
        KEY_EVENT,
        ENABLE_ECHO_INPUT,
        ENABLE_LINE_INPUT,
        ENABLE_PROCESSED_INPUT,
    )
else:
    import sys
    import select
    import termios
    import tty


class UnixKeyPoller:
    def __init__(self):
        try:
            self.stdin = sys.stdin.fileno()
            self.tattr = termios.tcgetattr(self.stdin)
            tty.setcbreak(self.stdin, termios.TCSANOW)
        except termios.error:
            pass
Repository: locustio/locust
File: locust/contrib/postgres.py (Python)

Code before:
from locust import TaskSet, User, events

import time

import psycopg


def create_conn(conn_string):
    return psycopg.connect(conn_string)


def execute_query(conn_string, query):
    db_conn = create_conn(conn_string)
    return db_conn.cursor().execute(query)


class PostgresClient:
    def __getattr__(self, name):
        def request_handler(*args, **kwargs):
            start_time = time.time()
            try:
                execute_query(*args, **kwargs)
                response_time = int((time.time() - start_time) * 1000)
                events.request.fire(
                    request_type="postgres_success",
                    name=name,
                    response_time=response_time,
                    response_length=0,
                )
            except Exception as e:
                response_time = int((time.time() - start_time) * 1000)
                events.request.fire(
                    request_type="postgres_failure",
                    name=name,
                    response_time=response_time,
                    response_length=0,
                    exception=e,
                )
                print(f"error: {e}")

        return request_handler



Review comment (suggestion):
I dont think you need the whole `__getattr__` song and dance :)

Just define the execute_query method directly on the PostgresClient class.

Code after:
from locust import User, events

import time

import psycopg


class PostgresClient:
    def __init__(self, conn_string):
        self.connection = psycopg.connect(conn_string)

    def execute_query(self, query):
        start_time = time.time()
        try:
            self.connection.execute(query)
            response_time = int((time.time() - start_time) * 1000)
            events.request.fire(
                request_type="postgres_success",
                name="execute_query",
                response_time=response_time,
                response_length=0,
            )
        except Exception as e:
            response_time = int((time.time() - start_time) * 1000)
            events.request.fire(
                request_type="postgres_failure",
                name="execute_query",
                response_time=response_time,
                response_length=0,
                exception=e,
            )

    def close(self):
        self.connection.close()


class PostgresUser(User):
    abstract = True

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.client = PostgresClient(conn_string=self.conn_string)
Repository: locustio/locust
File: examples/postgres/locustfile.py (Python)

Code before:
from locust import TaskSet, between, task
from locust.contrib.postgres import PostgresUser

import os
import random


class UserTasks(TaskSet):
    @task
    def run_select_query(self):
        self.client.execute_query(
            "SELECT * FROM loadtesting.invoice WHERE amount > 500",
        )

    @task(3)
    def run_update_query(self):
        random_amount = random.randint(1, 12)
        self.client.execute_query(
            f"UPDATE loadtesting.invoice SET amount={random_amount} WHERE amount < 10",
        )


class PostgresLocust(PostgresUser):
    tasks = [UserTasks]
    min_wait = 0
    max_wait = 3
    wait_time = between(min_wait, max_wait)

    # Use environment variables or default values
    PGHOST = os.getenv("PGHOST", "localhost")
    PGPORT = os.getenv("PGPORT", "5432")
    PGDATABASE = os.getenv("PGDATABASE", "postgres")
    PGUSER = os.getenv("PGUSER", "postgres")
    PGPASSWORD = os.getenv("PGPASSWORD", "postgres")

    conn_string = f"postgresql://{PGUSER}:{PGPASSWORD}@{PGHOST}:{PGPORT}/{PGDATABASE}"


Review comment (refactor):
remove the wait_time stuff and just leave it as the default, to simplify the example.

You can skip the taskset and put the tasks directly under the user too.

Code after:
from locust import TaskSet, between, task
from locust.contrib.postgres import PostgresUser

import os
import random


class PostgresLocust(PostgresUser):
    @task
    def run_select_query(self):
        self.client.execute_query(
            "SELECT * FROM loadtesting.invoice WHERE amount > 500",
        )

    @task
    def run_update_query(self):
        random_amount = random.randint(1, 12)
        self.client.execute_query(
            f"UPDATE loadtesting.invoice SET amount={random_amount} WHERE amount < 10",
        )

    # Use environment variables or default values
    PGHOST = os.getenv("PGHOST", "localhost")
    PGPORT = os.getenv("PGPORT", "5432")
    PGDATABASE = os.getenv("PGDATABASE", "test_db")
    PGUSER = os.getenv("PGUSER", "postgres")
    PGPASSWORD = os.getenv("PGPASSWORD", "postgres")

    conn_string = f"postgresql://{PGUSER}:{PGPASSWORD}@{PGHOST}:{PGPORT}/{PGDATABASE}"
Repository: locustio/locust
File: locust/html.py (Python)

Code before:
        template_args={
            "is_report": True,
            "requests_statistics": [stat.to_dict(escape_string_values=True) for stat in requests_statistics],
            "failures_statistics": [stat.to_dict() for stat in failures_statistics],
            "exceptions_statistics": [stat for stat in exceptions_statistics],
            "response_time_statistics": [
                {
                    "name": escape(stat.name),
                    "method": escape(stat.method or ""),
                    **{
                        str(percentile): stat.get_response_time_percentile(percentile)
                        for percentile in PERCENTILES_FOR_HTML_REPORT
                    },
                }
                for stat in requests_statistics
            ],
            "start_time": start_time,
            "end_time": end_time,
            "duration": format_duration(stats.start_time, end_ts),
            "host": escape(str(host)),
            "history": history,
            "show_download_link": show_download_link,
            "locustfile": escape(str(environment.locustfile)),
            "tasks": task_data,
            "percentiles_to_chart": stats_module.PERCENTILES_TO_CHART,
            "profile": escape(str(environment.profile)),
        },
        theme=theme,
    )


Review comment (suggestion):
Can you double check this? I think if no profile is set this gets sent to the frontend as a string `"None"`. I think we'll want to return `None` or an empty string here

Code after:
        template_args={
            "is_report": True,
            "requests_statistics": [stat.to_dict(escape_string_values=True) for stat in requests_statistics],
            "failures_statistics": [stat.to_dict() for stat in failures_statistics],
            "exceptions_statistics": [stat for stat in exceptions_statistics],
            "response_time_statistics": [
                {
                    "name": escape(stat.name),
                    "method": escape(stat.method or ""),
                    **{
                        str(percentile): stat.get_response_time_percentile(percentile)
                        for percentile in PERCENTILES_FOR_HTML_REPORT
                    },
                }
                for stat in requests_statistics
            ],
            "start_time": start_time,
            "end_time": end_time,
            "duration": format_duration(stats.start_time, end_ts),
            "host": escape(str(host)),
            "history": history,
            "show_download_link": show_download_link,
            "locustfile": escape(str(environment.locustfile)),
            "tasks": task_data,
            "percentiles_to_chart": stats_module.PERCENTILES_TO_CHART,
            "profile": escape(str(environment.profile)) if environment.profile else None,
        },
        theme=theme,
    )
Repository: logto-io/logto
File: packages/shared/src/utils/phone.ts (TypeScript)

Code before:
import { parsePhoneNumberWithError } from 'libphonenumber-js/mobile';
import type { E164Number } from 'libphonenumber-js/mobile';

export { ParseError } from 'libphonenumber-js/mobile';

function validateE164Number(value: string): asserts value is E164Number {
  if (value && !value.startsWith('+')) {
    throw new TypeError(`Invalid E164Number: ${value}`);
  }
}

const parseE164Number = (value: string): E164Number | '' => {
  // If typeof `value` is string and `!value` is true, then `value` is an empty string.
  if (!value) {
    return '';
  }

  const result = value.startsWith('+') ? value : `+${value}`;
  validateE164Number(result);
  return result;
};

export const parseE164PhoneNumberWithError = (value: string) =>
  parsePhoneNumberWithError(parseE164Number(value));

/**
 * Parse phone number to number string.
 * E.g. +1 (650) 253-0000 -> 16502530000
 */
export const parsePhoneNumber = (phone: string) => {
  try {
    return parseE164PhoneNumberWithError(phone).number.slice(1);
  } catch {
    console.error(`Invalid phone number: ${phone}`);
    return phone;

Review comment (suggestion):
The comment seems unnecessary.  the input type value is declared as a string already. 
 
```
if (!value) {
 return value;
 }
```

Code after:
import { parsePhoneNumberWithError, type PhoneNumber } from 'libphonenumber-js';

/**
 * Parse phone number to E.164 format.
 * This method will add a leading `+` sign if the phone number does not start with it.
 */
export const parseE164Number = (phone: string) => {
  if (!phone || phone.startsWith('+')) {
    return phone;
  }

  return `+${phone}`;
};

/**
 * Parse phone number to number string.
 * This method will strip the:
 *  - leading `+` sign of the country code if any,
 *  - the leading `0` of the phone number if any,
 *  - non-digit characters if any.
 *
 * @params phone - The phone number to parse.
 * @params throwError - If true, throw an error if the phone number is invalid. Otherwise, return the original phone number.
 *
 * @example
 * +1 (650) 253-0000 -> 16502530000
 *
 * @example
 * +61 0412 345 678 -> 61412345678
 */
export const parsePhoneNumber = (phone: string, throwError?: true) => {
  try {
    return parsePhoneNumberWithError(parseE164Number(phone)).number.slice(1);
  } catch (error: unknown) {
    if (throwError) {