ronantakizawa/codereview-bench huggingface dataset:
Repository: home-assistant/core
File: tests/util/test_file.py (Python)

Code before:
    test_dir = tmpdir.mkdir("files")
    test_file = Path(test_dir / "test.json")

    with (
        pytest.raises(WriteError),
        patch("homeassistant.util.file.os.remove", side_effect=OSError),
        patch("homeassistant.util.file.os.replace", side_effect=OSError),
    ):
        write_utf8_file(test_file, '{"some":"data"}', False)

    assert "File replacement cleanup failed" in caplog.text


@pytest.mark.parametrize("func", [write_utf8_file, write_utf8_file_atomic])
def test_write_utf8_file_with_non_ascii_content(tmp_path: Path, func) -> None:
    """Test files with non-ASCII content can be written even when locale is ASCII."""
    test_dir = tmp_path / "files"
    test_dir.mkdir()
    test_file = test_dir / "test.json"
    non_ascii_data = '{"name":"自动化","emoji":"🏠"}'

    with patch("locale.getpreferredencoding", return_value="ascii"):
        func(test_file, non_ascii_data, False)

    with open(test_file, encoding="utf-8") as fh:
        assert fh.read() == non_ascii_data


def test_write_utf8_file_atomic_fails(tmpdir: py.path.local) -> None:
    """Test OSError from write_utf8_file_atomic is rethrown as WriteError."""
    test_dir = tmpdir.mkdir("files")
    test_file = Path(test_dir / "test.json")

    with (
        pytest.raises(WriteError),
        patch("homeassistant.util.file.AtomicWriter.open", side_effect=OSError),
    ):
        write_utf8_file_atomic(test_file, '{"some":"data"}', False)

    assert not os.path.exists(test_file)


Review comment (suggestion):
```suggestion
    assert file_text == non_ascii_data
```

Code after:
    test_dir = tmpdir.mkdir("files")
    test_file = Path(test_dir / "test.json")

    with (
        pytest.raises(WriteError),
        patch("homeassistant.util.file.os.remove", side_effect=OSError),
        patch("homeassistant.util.file.os.replace", side_effect=OSError),
    ):
        write_utf8_file(test_file, '{"some":"data"}', False)

    assert "File replacement cleanup failed" in caplog.text


@pytest.mark.parametrize("func", [write_utf8_file, write_utf8_file_atomic])
def test_write_utf8_file_with_non_ascii_content(tmp_path: Path, func) -> None:
    """Test files with non-ASCII content can be written even when locale is ASCII."""
    test_file = tmp_path / "test.json"
    non_ascii_data = '{"name":"自动化","emoji":"🏠"}'

    with patch("locale.getpreferredencoding", return_value="ascii"):
        func(test_file, non_ascii_data, False)

    file_text = test_file.read_text(encoding="utf-8")
    assert file_text == non_ascii_data


def test_write_utf8_file_atomic_fails(tmpdir: py.path.local) -> None:
    """Test OSError from write_utf8_file_atomic is rethrown as WriteError."""
    test_dir = tmpdir.mkdir("files")
    test_file = Path(test_dir / "test.json")

    with (
        pytest.raises(WriteError),
        patch("homeassistant.util.file.AtomicWriter.open", side_effect=OSError),
    ):
        write_utf8_file_atomic(test_file, '{"some":"data"}', False)

    assert not os.path.exists(test_file)
Repository: home-assistant/core
File: tests/components/evohome/test_services.py (Python)

Code before:
                ATTR_DURATION: {"minutes": 135},
            },
            target={ATTR_ENTITY_ID: zone_id},
            blocking=True,
        )

        mock_fcn.assert_awaited_once_with(
            19.5, until=datetime(2024, 7, 10, 14, 15, tzinfo=UTC)
        )


@pytest.mark.parametrize("install", ["default"])
async def test_zone_clear_zone_override_with_ctl_id(  # ServiceValidationError
    hass: HomeAssistant,
    ctl_id: str,
) -> None:
    """Test calling a zone service with a non-zone entity_id fails."""

    with pytest.raises(ServiceValidationError) as excinfo:
        await hass.services.async_call(
            DOMAIN,
            EvoService.CLEAR_ZONE_OVERRIDE,
            {},
            target={ATTR_ENTITY_ID: ctl_id},
            blocking=True,
        )

    assert excinfo.value.translation_key == "zone_only_service"


Review comment (suggestion):
Can you maybe parametrize this to test both service calls?
```python
@pytest.mark.parametrize("install", ["default"])
@pytest.mark.parametrize("service_name", [EvoService.CLEAR_ZONE_OVERRIDE, EvoService.SET_ZONE_OVERRIDE])
async def test_zone_clear_zone_override_with_ctl_id(  # ServiceValidationError
    hass: HomeAssistant,
    ctl_id: str,
    service_name: str,
) -> None:
```

Code after:
                ATTR_SETPOINT: 19.5,
                ATTR_DURATION: {"minutes": 135},
            },
            target={ATTR_ENTITY_ID: zone_id},
            blocking=True,
        )

        mock_fcn.assert_awaited_once_with(
            19.5, until=datetime(2024, 7, 10, 14, 15, tzinfo=UTC)
        )


@pytest.mark.parametrize("install", ["default"])
@pytest.mark.parametrize(
    ("service", "service_data"),
    [
        (EvoService.CLEAR_ZONE_OVERRIDE, {}),
        (EvoService.SET_ZONE_OVERRIDE, {ATTR_SETPOINT: 19.5}),
    ],
)
async def test_zone_services_with_ctl_id(
    hass: HomeAssistant,
    ctl_id: str,
    service: EvoService,
    service_data: dict[str, Any],
) -> None:
    """Test calling zone-only services with a non-zone entity_id fail."""

    with pytest.raises(ServiceValidationError) as excinfo:
        await hass.services.async_call(
            DOMAIN,
            service,
            service_data,
            target={ATTR_ENTITY_ID: ctl_id},
            blocking=True,
        )

    assert excinfo.value.translation_key == "zone_only_service"
Repository: home-assistant/core
File: homeassistant/components/aws_s3/diagnostics.py (Python)

Code before:
import dataclasses
from typing import Any

from homeassistant.components.backup import (
    DATA_MANAGER as BACKUP_DATA_MANAGER,
    BackupManager,
)
from homeassistant.components.diagnostics import async_redact_data
from homeassistant.core import HomeAssistant

from .const import CONF_ACCESS_KEY_ID, CONF_BUCKET, CONF_SECRET_ACCESS_KEY, DOMAIN
from .coordinator import S3ConfigEntry
from .helpers import async_list_backups_from_s3

TO_REDACT = (CONF_ACCESS_KEY_ID, CONF_SECRET_ACCESS_KEY)


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant,
    entry: S3ConfigEntry,
) -> dict[str, Any]:
    """Return diagnostics for a config entry."""
    coordinator = entry.runtime_data
    backup_manager: BackupManager = hass.data[BACKUP_DATA_MANAGER]
    bucket = entry.data[CONF_BUCKET]
    backups = await async_list_backups_from_s3(coordinator.client, bucket)

    data = {
        "coordinator_data": dataclasses.asdict(coordinator.data),
        "config": {
            **entry.data,
            **entry.options,
        },
        "backup_agents": [
            {"name": agent.name}
            for agent in backup_manager.backup_agents.values()
            if agent.domain == DOMAIN
        ],
        "backup": [backup.as_dict() for backup in backups],
    }

    return async_redact_data(data, TO_REDACT)


Review comment (suggestion):
```suggestion
    bucket = entry.data[CONF_BUCKET]
    backups = await async_list_backups_from_s3(coordinator.client, entry.data[CONF_BUCKET])
```
Single-use of bucket.

Code after:
import dataclasses
from typing import Any

from homeassistant.components.backup import (
    DATA_MANAGER as BACKUP_DATA_MANAGER,
    BackupManager,
)
from homeassistant.components.diagnostics import async_redact_data
from homeassistant.core import HomeAssistant

from .const import (
    CONF_ACCESS_KEY_ID,
    CONF_BUCKET,
    CONF_PREFIX,
    CONF_SECRET_ACCESS_KEY,
    DOMAIN,
)
from .coordinator import S3ConfigEntry
from .helpers import async_list_backups_from_s3

TO_REDACT = (CONF_ACCESS_KEY_ID, CONF_SECRET_ACCESS_KEY)


async def async_get_config_entry_diagnostics(
    hass: HomeAssistant,
    entry: S3ConfigEntry,
) -> dict[str, Any]:
    """Return diagnostics for a config entry."""
    coordinator = entry.runtime_data
    backup_manager: BackupManager = hass.data[BACKUP_DATA_MANAGER]
    backups = await async_list_backups_from_s3(
        coordinator.client,
        bucket=entry.data[CONF_BUCKET],
        prefix=entry.data.get(CONF_PREFIX, ""),
    )

    data = {
        "coordinator_data": dataclasses.asdict(coordinator.data),
        "config": {
            **entry.data,
            **entry.options,
        },
        "backup_agents": [
            {"name": agent.name}
            for agent in backup_manager.backup_agents.values()
            if agent.domain == DOMAIN
        ],
        "backup": [backup.as_dict() for backup in backups],
    }

    return async_redact_data(data, TO_REDACT)
Repository: honojs/hono
File: src/middleware/compress/index.ts (TypeScript)

Code before:
import * as WebStreams from 'node:stream/web'
import type { Context } from '../../context'
import type { Next } from '../../hono'
const { CompressionStream } = WebStreams as any

interface CompressionOptions {
    encoding?: 'gzip' | 'deflate'
}

export const compress = (options?: CompressionOptions) => {
  return async (ctx: Context, next: Next) => {
    await next()
    const accepted = ctx.req.headers.get('Accept-Encoding')
    const pattern = options?.encoding ?? /gzip|deflate/
    const match = accepted?.match(pattern)
    if (!accepted || !match || !ctx.res.body) {
        return
    }
    const encoding = match[0]
    const stream = new CompressionStream(encoding)
    ctx.res = new Response(ctx.res.body.pipeThrough(stream), ctx.res.clone())
    ctx.res.headers.set('Content-Encoding', encoding)
  }
}

Review comment (suggestion):
@tani 

Hey, we must not use **node** module. Only use web standard API available on Cloudflare and Deno.

Code after:
import type { Context } from '../../context'
import type { Next } from '../../hono'

interface CompressionOptions {
    encoding?: 'gzip' | 'deflate'
}

export const compress = (options?: CompressionOptions) => {
  return async (ctx: Context, next: Next) => {
    await next()
    const accepted = ctx.req.headers.get('Accept-Encoding')
    const pattern = options?.encoding ?? /gzip|deflate/
    const match = accepted?.match(pattern)
    if (!accepted || !match || !ctx.res.body) {
        return
    }
    const encoding = match[0]
    const stream = new CompressionStream(encoding)
    ctx.res = new Response(ctx.res.body.pipeThrough(stream), ctx.res.clone())
    ctx.res.headers.set('Content-Encoding', encoding)
  }
}
Repository: honojs/hono
File: src/middleware/compress/index.test.ts (TypeScript)

Code before:
import { Hono } from '../../hono'
import { compress } from '.'

describe('Parse Compress Middleware', () => {
  
  if (parseInt(process.versions.node.split('.')[0]) < 17) {
    it('node version', () => {
      expect(parseInt(process.versions.node.split('.')[0])).toBeLessThan(17)
    })
    return
  }

  const app = new Hono()

  app.use('*', compress())
  app.get('/hello', async (ctx) => {
    return ctx.text('hello')
  })

  it('gzip', async () => {
    const req = new Request('http://localhost/hello', {
      method: 'GET',
      headers: new Headers({ 'Accept-Encoding': 'gzip' }),
    })
    const res = await app.request(req)
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(res.headers.get('Content-Encoding')).toEqual('gzip')
  })
  
  it('deflate', async () => {

Review comment (suggestion):
@tani 

I think this version check is not needed. We don't use node `node:stream/web`.

Code after:
import { Hono } from '../../hono'
import { compress } from '.'

describe('Parse Compress Middleware', () => {

  const app = new Hono()

  app.use('*', compress())
  app.get('/hello', async (ctx) => {
    return ctx.text('hello')
  })

  it('gzip', async () => {
    const req = new Request('http://localhost/hello', {
      method: 'GET',
      headers: new Headers({ 'Accept-Encoding': 'gzip' }),
    })
    const res = await app.request(req)
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(res.headers.get('Content-Encoding')).toEqual('gzip')
  })
  
  it('deflate', async () => {
    const req = new Request('http://localhost/hello', {
      method: 'GET',
      headers: new Headers({ 'Accept-Encoding': 'deflate' }),
    })
    const res = await app.request(req)
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
Repository: honojs/hono
File: deno_dist/hono.ts (TypeScript)

Code before:
    return this
  }

  notFound(handler: NotFoundHandler): Hono<E, P> {
    this.notFoundHandler = handler
    return this
  }

  private addRoute(method: string, path: string, handler: Handler<string, E>): void {
    method = method.toUpperCase()
    if (this._tempPath) {
      path = mergePath(this._tempPath, path)
    }
    this.router.add(method, path, handler)
    const r: Route<E> = { path: path, method: method, handler: handler }
    this.routes.push(r)
  }

  private matchRoute(method: string, path: string) {
    return this.router.match(method, path)
  }

  private async dispatch(
    request: Request,
    eventOrExecutionCtx?: ExecutionContext | FetchEvent,
    Environment?: E['Bindings']
  ): Promise<Response> {
    const path = getPathFromURL(request.url, this.strict)
    const method = request.method

    const result = this.matchRoute(method, path)
    request.paramData = result?.params

    const handlers = result ? result.handlers : [this.notFoundHandler]

    const c = new HonoContext<string, E>(
      request,
      Environment,
      eventOrExecutionCtx,
      this.notFoundHandler
    )

    const composed = compose<HonoContext<string, E>>(
      handlers,
      this.errorHandler,
      this.notFoundHandler
    )
    let context: HonoContext<string, E>
    try {
      context = await composed(c)
      if (!context.finalized) {

Review comment (suggestion):
Looks like a typo.

```suggestion
    environment?: E['Bindings']
```

Code after:
    return this
  }

  notFound(handler: NotFoundHandler): Hono<E, P> {
    this.notFoundHandler = handler
    return this
  }

  private addRoute(method: string, path: string, handler: Handler<string, E>): void {
    method = method.toUpperCase()
    if (this._tempPath) {
      path = mergePath(this._tempPath, path)
    }
    this.router.add(method, path, handler)
    const r: Route<E> = { path: path, method: method, handler: handler }
    this.routes.push(r)
  }

  private matchRoute(method: string, path: string) {
    return this.router.match(method, path)
  }

  private async dispatch(
    request: Request,
    eventOrExecutionCtx?: ExecutionContext | FetchEvent,
    env?: E['Bindings']
  ): Promise<Response> {
    const path = getPathFromURL(request.url, this.strict)
    const method = request.method

    const result = this.matchRoute(method, path)
    request.paramData = result?.params

    const handlers = result ? result.handlers : [this.notFoundHandler]

    const c = new HonoContext<string, E>(request, env, eventOrExecutionCtx, this.notFoundHandler)

    const composed = compose<HonoContext<string, E>>(
      handlers,
      this.errorHandler,
      this.notFoundHandler
    )
    let context: HonoContext<string, E>
    try {
      context = await composed(c)
      if (!context.finalized) {
        throw new Error(
          'Context is not finalized. You may forget returning Response object or `await next()`'
        )
      }
    } catch (err) {
Repository: honojs/hono
File: deno_dist/validator/validator.ts (TypeScript)

Code before:
      },
    ]
    this.sanitizers = []
    this._optional = false
    this.isArray = options.isArray || false
  }

  private _nested = () => (this.baseKeys.length ? true : false)

  addRule(func: RuleFunc): this
  addRule(name: string, func: RuleFunc): this
  addRule(arg: string | RuleFunc, func?: RuleFunc) {
    if (typeof arg === 'string' && func) {
      this.rules.push({ name: arg, func, type: 'value' })
    } else if (arg instanceof Function) {
      this.rules.push({ name: arg.name, func: arg, type: 'value' })
    }
    return this
  }

  addSanitizer = (sanitizer: Sanitizer) => {
    this.sanitizers.push(sanitizer)
    return this
  }

  sanitizeValue = (value: Type) => (
    this.sanitizers.reduce(
      (acc, sanitizer) => sanitizer(acc),
      value
    )
  )

  message = (text: string) => {
    const len = this.rules.length
    if (len >= 1) {
      this.rules[len - 1].customMessage = text
    }
    return this
  }

  isRequired = () => {
    return this.addRule('isRequired', (value: unknown) => {
      if (value !== undefined && value !== null && value !== '') return true
      return false
    })
  }

  isOptional = () => {
    this._optional = true
    return this.addRule('isOptional', () => true)
  }

Review comment (security):
How about making it "private"? I think it's better that users do not use `sanitizeValue` directly. And in that case, it would be better to write this function much later. Like above the `validateRule`.

Code after:
      },
    ]
    this.sanitizers = []
    this._optional = false
    this.isArray = options.isArray || false
  }

  private _nested = () => (this.baseKeys.length ? true : false)

  addRule(func: RuleFunc): this
  addRule(name: string, func: RuleFunc): this
  addRule(arg: string | RuleFunc, func?: RuleFunc) {
    if (typeof arg === 'string' && func) {
      this.rules.push({ name: arg, func, type: 'value' })
    } else if (arg instanceof Function) {
      this.rules.push({ name: arg.name, func: arg, type: 'value' })
    }
    return this
  }

  addSanitizer = (sanitizer: Sanitizer) => {
    this.sanitizers.push(sanitizer)
    return this
  }

  message = (text: string) => {
    const len = this.rules.length
    if (len >= 1) {
      this.rules[len - 1].customMessage = text
    }
    return this
  }

  isRequired = () => {
    return this.addRule('isRequired', (value: unknown) => {
      if (value !== undefined && value !== null && value !== '') return true
      return false
    })
  }

  isOptional = () => {
    this._optional = true
    return this.addRule('isOptional', () => true)
  }

  isEqual = (comparison: unknown) => {
    return this.addRule('isEqual', (value: unknown) => {
      return value === comparison
    })
  }
Repository: honojs/hono
File: src/middleware/cors/index.ts (TypeScript)

Code before:
import type { MiddlewareHandler } from '../../types'

type CORSOptions = {
  origin: string | string[] | ((origin: string) => string | undefined | null)
  allowMethods?: string[]
  allowHeaders?: string[]
  maxAge?: number
  credentials?: boolean
  exposeHeaders?: string[]
}

export const cors = (options?: CORSOptions): MiddlewareHandler => {
  const defaults: CORSOptions = {
    origin: '*',
    allowMethods: ['GET', 'HEAD'],
    allowHeaders: [],
    exposeHeaders: [],
  }
  const opts = {
    ...defaults,
    ...options,
  }

  const findAllowOrigin = ((optsOrigin) => {
    if (typeof optsOrigin === 'string') {
      return () => optsOrigin
    } else if (typeof optsOrigin === 'function') {
      return optsOrigin
    } else {
      return (origin: string) => (optsOrigin.includes(origin) ? origin : optsOrigin[0])
    }
  })(opts.origin)

  return async (c, next) => {
    
    function set(key: string, value: string) {
      c.res.headers.append(key, value)
    }

    const allowOrigin = findAllowOrigin(c.req.headers.get('origin') || '')

Review comment (suggestion):
I'm reconsidering. How about this line? I think the current implementation is OK.

```ts
['GET', 'HEAD', 'PUT', 'POST', 'DELETE', 'PATCH']
```

Indeed, it is more secure to limit `GET` and `HEAD`, but this is breaking changes. And it may be common to allow the above six methods by default. For example, Express cors implementation:

https://github.com/expressjs/cors/blob/f038e7722838fd83935674aa8c5bf452766741fb/lib/index.js#L10

Code after:
import type { MiddlewareHandler } from '../../types'

type CORSOptions = {
  origin: string | string[] | ((origin: string) => string | undefined | null)
  allowMethods?: string[]
  allowHeaders?: string[]
  maxAge?: number
  credentials?: boolean
  exposeHeaders?: string[]
}

export const cors = (options?: CORSOptions): MiddlewareHandler => {
  const defaults: CORSOptions = {
    origin: '*',
    allowMethods: ['GET', 'HEAD', 'PUT', 'POST', 'DELETE', 'PATCH'],
    allowHeaders: [],
    exposeHeaders: [],
  }
  const opts = {
    ...defaults,
    ...options,
  }

  const findAllowOrigin = ((optsOrigin) => {
    if (typeof optsOrigin === 'string') {
      return () => optsOrigin
    } else if (typeof optsOrigin === 'function') {
      return optsOrigin
    } else {
      return (origin: string) => (optsOrigin.includes(origin) ? origin : optsOrigin[0])
    }
  })(opts.origin)

  return async (c, next) => {
    
    function set(key: string, value: string) {
      c.res.headers.append(key, value)
    }

    const allowOrigin = findAllowOrigin(c.req.headers.get('origin') || '')
Repository: honojs/hono
File: src/client/client.test.ts (TypeScript)

Code before:
    Bindings: {
      TOKEN: string
    }
  }

  it('Should have correct types', async () => {
    const api = new Hono<Env>().get('/search', (c) => c.jsonT({ ok: true }))
    const app = new Hono<Env>().route('/api', api)
    type AppType = typeof app
    const client = hc<AppType>('http://localhost')
    const res = await client.api.search.$get()
    const data = await res.json()
    type verify = Expect<Equal<boolean, typeof data.ok>>
    expect(data.ok).toBe(true)
  })

  describe('Multiple endpoints', () => {
    const api = new Hono()
      .get('/foo', (c) => c.jsonT({ foo: '' }))
      .post('/bar', (c) => c.jsonT({ bar: 0 }))
    const app = new Hono().route('/api', api)
    type AppType = typeof app
    const client = hc<typeof app>('http://localhost')

    it('Should return correct types - GET /api/foo', async () => {
      type verify1 = Expect<NotEqual<typeof client.api.foo.$get, any>>
      const res = await client.api.foo.$get()
      const data = await res.json()
      type verify2 = Expect<Equal<string, typeof data.foo>>
    })

    it('Should return correct types - POST /api/bar', async () => {
      type verify1 = Expect<NotEqual<typeof client.api.bar.$post, any>>
      const res = await client.api.bar.$post()
      const data = await res.json()
      type verify2 = Expect<Equal<number, typeof data.bar>>
    })
  })
})


Review comment (bug):
Why would you assert that `typeof client.api.foo.$get` is `NotEqual` to `any`? In the next line, you get `res` with the correct type, so I suppose you can compare `typeof client.api.foo.$get` with that type. Correct me if I'm wrong.

Code after:
    Bindings: {
      TOKEN: string
    }
  }

  it('Should have correct types', async () => {
    const api = new Hono<Env>().get('/search', (c) => c.jsonT({ ok: true }))
    const app = new Hono<Env>().route('/api', api)
    type AppType = typeof app
    const client = hc<AppType>('http://localhost')
    const res = await client.api.search.$get()
    const data = await res.json()
    type verify = Expect<Equal<boolean, typeof data.ok>>
    expect(data.ok).toBe(true)
  })

  describe('Multiple endpoints', () => {
    const api = new Hono()
      .get('/foo', (c) => c.jsonT({ foo: '' }))
      .post('/bar', (c) => c.jsonT({ bar: 0 }))
    const app = new Hono().route('/api', api)
    type AppType = typeof app
    const client = hc<typeof app>('http://localhost')

    it('Should return correct types - GET /api/foo', async () => {
      const res = await client.api.foo.$get()
      const data = await res.json()
      type verify = Expect<Equal<string, typeof data.foo>>
    })

    it('Should return correct types - POST /api/bar', async () => {
      const res = await client.api.bar.$post()
      const data = await res.json()
      type verify = Expect<Equal<number, typeof data.bar>>
    })
  })
})