ronantakizawa/codereview-bench huggingface dataset:
Repository: fastify/fastify
File: lib/reply.js (JavaScript)

Code before:
}

Reply.prototype.serializer = function (fn) {
  this[kReplySerializer] = fn
  return this
}

Reply.prototype.type = function (type) {
  this[kReplyHeaders]['content-type'] = type
  return this
}

Reply.prototype.redirect = function (code, url) {
  if (typeof code === 'string') {
    url = code
    code = this[kReplyHasStatusCode] ? this.raw.statusCode : 302
  }

  return this.header('location', url).code(code).send()
}

Reply.prototype.callNotFound = function () {
  notFound(this)
  return this
}

// TODO: should be removed in fastify@5
Reply.prototype.getResponseTime = function () {
  FSTDEP020()

  let responseTime = 0

  if (this[kReplyStartTime] !== undefined) {
    responseTime = (this[kReplyEndTime] || now()) - this[kReplyStartTime]
  }

  return responseTime
}

// Make reply a thenable, so it could be used with async/await.
// See
// - https://github.com/fastify/fastify/issues/1864 for the discussions
// - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then for the signature
Reply.prototype.then = function (fulfilled, rejected) {
  if (this.sent) {
    fulfilled()
    return
  }

  eos(this.raw, (err) => {
    // We must not treat ERR_STREAM_PREMATURE_CLOSE as

Review comment (suggestion):
Can you add a test to check this is correctly emitted. You can modify one of the existing tests for `getResponseTime()`.

Code after:
  this[kReplySerializer] = fn
  return this
}

Reply.prototype.type = function (type) {
  this[kReplyHeaders]['content-type'] = type
  return this
}

Reply.prototype.redirect = function (code, url) {
  if (typeof code === 'string') {
    url = code
    code = this[kReplyHasStatusCode] ? this.raw.statusCode : 302
  }

  return this.header('location', url).code(code).send()
}

Reply.prototype.callNotFound = function () {
  notFound(this)
  return this
}

// TODO: should be removed in fastify@5
Reply.prototype.getResponseTime = function () {
  FSTDEP020()

  return this.elapsedTime
}

// Make reply a thenable, so it could be used with async/await.
// See
// - https://github.com/fastify/fastify/issues/1864 for the discussions
// - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then for the signature
Reply.prototype.then = function (fulfilled, rejected) {
  if (this.sent) {
    fulfilled()
    return
  }

  eos(this.raw, (err) => {
    // We must not treat ERR_STREAM_PREMATURE_CLOSE as
    // an error because it is created by eos, not by the stream.
    if (err && err.code !== 'ERR_STREAM_PREMATURE_CLOSE') {
      if (rejected) {
        rejected(err)
      } else {
        this.log && this.log.warn('unhandled rejection on reply.then')
      }
    } else {
      fulfilled()
Repository: fastify/fastify
File: test/internals/reply.test.js (JavaScript)

Code before:
    t.ok(reply.elapsedTime > 0)
    t.end()
  })

  fastify.inject({ method: 'GET', url: '/' })
})

test('reply.elapsedTime should return the time since a request started while inflight', t => {
  t.plan(1)
  const fastify = Fastify()
  fastify.route({
    method: 'GET',
    url: '/',
    handler: (req, reply) => {
      reply.send('hello world')
    }
  })

  fastify.addHook('preValidation', (req, reply, done) => {
    t.not(reply.elapsedTime, reply.elapsedTime)
    done()
  })

  fastify.addHook('onResponse', (req, reply) => {
    t.end()
  })

  fastify.inject({ method: 'GET', url: '/' })
})

test('reply.elapsedTime should return the same value after a request is finished', t => {
  t.plan(1)
  const fastify = Fastify()
  fastify.route({
    method: 'GET',
    url: '/',
    handler: (req, reply) => {
      reply.send('hello world')
    }
  })

  fastify.addHook('onResponse', (req, reply) => {
    t.equal(reply.elapsedTime, reply.elapsedTime)
    t.end()
  })

  fastify.inject({ method: 'GET', url: '/' })
})

test('reply should use the custom serializer', t => {
  t.plan(4)

Review comment (refactor):
Maybe refactor this test to compare the `elapsedTime` within `preValidation` and later within `onResponse` to verify the latter is greater than the former?

Code after:
  fastify.inject({ method: 'GET', url: '/' })
})

test('reply.elapsedTime should return the time since a request started while inflight', t => {
  t.plan(1)
  const fastify = Fastify()
  fastify.route({
    method: 'GET',
    url: '/',
    handler: (req, reply) => {
      reply.send('hello world')
    }
  })

  let preValidationElapsedTime

  fastify.addHook('preValidation', (req, reply, done) => {
    preValidationElapsedTime = reply.elapsedTime

    done()
  })

  fastify.addHook('onResponse', (req, reply) => {
    t.ok(reply.elapsedTime > preValidationElapsedTime)
    t.end()
  })

  fastify.inject({ method: 'GET', url: '/' })
})

test('reply.elapsedTime should return the same value after a request is finished', t => {
  t.plan(1)
  const fastify = Fastify()
  fastify.route({
    method: 'GET',
    url: '/',
    handler: (req, reply) => {
      reply.send('hello world')
    }
  })

  fastify.addHook('onResponse', (req, reply) => {
    t.equal(reply.elapsedTime, reply.elapsedTime)
    t.end()
  })

  fastify.inject({ method: 'GET', url: '/' })
})

test('reply should use the custom serializer', t => {
  t.plan(4)
Repository: fastify/fastify
File: lib/reply.js (JavaScript)

Code before:
  this[kReplyStartTime] = undefined
  this.log = log
}
Reply.props = []

Object.defineProperties(Reply.prototype, {
  [kRouteContext]: {
    get () {
      return this.request[kRouteContext]
    }
  },
  // TODO: remove once v5 is done
  // Is temporary to avoid constant conflicts between `next` and `main`
  context: {
    get () {
      FSTDEP019()
      return this.request[kRouteContext]
    }
  },
  elapsedTime: {
    get () {
      let elapsedTime = 0

      if (this[kReplyStartTime] !== undefined) {
        elapsedTime = (this[kReplyEndTime] || now()) - this[kReplyStartTime]
      }

      return elapsedTime
    }
  },
  server: {
    get () {
      return this.request[kRouteContext].server
    }
  },
  sent: {
    enumerable: true,
    get () {
      // We are checking whether reply was hijacked or the response has ended.
      return (this[kReplyHijacked] || this.raw.writableEnded) === true
    },
    set (value) {
      FSTDEP010()

      if (value !== true) {
        throw new FST_ERR_REP_SENT_VALUE()
      }

      // We throw only if sent was overwritten from Fastify
      if (this.sent && this[kReplyHijacked]) {
        throw new FST_ERR_REP_ALREADY_SENT(this.request.url, this.request.method)

Review comment (suggestion):
```suggestion
    get () {
      if (this[kReplyStartTime] === undefined) {
        return 0
      }
      return (this[kReplyEndTime] || now()) - this[kReplyStartTime]
    }
```

Code after:
  this[kReplyStartTime] = undefined
  this.log = log
}
Reply.props = []

Object.defineProperties(Reply.prototype, {
  [kRouteContext]: {
    get () {
      return this.request[kRouteContext]
    }
  },
  // TODO: remove once v5 is done
  // Is temporary to avoid constant conflicts between `next` and `main`
  context: {
    get () {
      FSTDEP019()
      return this.request[kRouteContext]
    }
  },
  elapsedTime: {
    get () {
      if (this[kReplyStartTime] === undefined) {
        return 0
      }
      return (this[kReplyEndTime] || now()) - this[kReplyStartTime]
    }
  },
  server: {
    get () {
      return this.request[kRouteContext].server
    }
  },
  sent: {
    enumerable: true,
    get () {
      // We are checking whether reply was hijacked or the response has ended.
      return (this[kReplyHijacked] || this.raw.writableEnded) === true
    },
    set (value) {
      FSTDEP010()

      if (value !== true) {
        throw new FST_ERR_REP_SENT_VALUE()
      }

      // We throw only if sent was overwritten from Fastify
      if (this.sent && this[kReplyHijacked]) {
        throw new FST_ERR_REP_ALREADY_SENT(this.request.url, this.request.method)
      }

      this[kReplyHijacked] = true
Repository: fastify/fastify
File: test/delete.test.js (JavaScript)

Code before:
    })
  })
})

test('shorthand - delete with application/json Content-Type header and null body', t => {
  t.plan(4)
  const fastify = require('..')()
  fastify.delete('/', {}, (req, reply) => {
    t.equal(req.body, null)
    reply.send(req.body)
  })
  fastify.inject({
    method: 'DELETE',
    url: '/',
    headers: { 'Content-Type': 'application/json' },
    body: 'null'
  }, (err, response) => {
    t.error(err)
    t.equal(response.statusCode, 200)
    t.same(response.payload.toString(), 'null')
  })
})

// https://github.com/fastify/fastify/issues/936
// Skip this test because this is an invalid request
test('shorthand - delete with application/json Content-Type header and without body', { skip: true }, t => {
  t.plan(4)
  const fastify = require('..')()
  fastify.delete('/', {}, (req, reply) => {
    t.equal(req.body, undefined)
    reply.send(req.body)
  })
  fastify.inject({
    method: 'DELETE',
    url: '/',
    headers: { 'Content-Type': 'application/json' },
    body: null
  }, (err, response) => {
    t.error(err)
    t.equal(response.statusCode, 200)
    t.same(response.payload.toString(), '')
  })
})


Review comment (suggestion):
```suggestion
test('shorthand - delete with application/json Content-Type header and without body', { skip: 'https://github.com/fastify/fastify/pull/5419' }, t => {
```

Code after:
    })
  })
})

test('shorthand - delete with application/json Content-Type header and null body', t => {
  t.plan(4)
  const fastify = require('..')()
  fastify.delete('/', {}, (req, reply) => {
    t.equal(req.body, null)
    reply.send(req.body)
  })
  fastify.inject({
    method: 'DELETE',
    url: '/',
    headers: { 'Content-Type': 'application/json' },
    body: 'null'
  }, (err, response) => {
    t.error(err)
    t.equal(response.statusCode, 200)
    t.same(response.payload.toString(), 'null')
  })
})

// https://github.com/fastify/fastify/issues/936
// Skip this test because this is an invalid request
test('shorthand - delete with application/json Content-Type header and without body', { skip: 'https://github.com/fastify/fastify/pull/5419' }, t => {
  t.plan(4)
  const fastify = require('..')()
  fastify.delete('/', {}, (req, reply) => {
    t.equal(req.body, undefined)
    reply.send(req.body)
  })
  fastify.inject({
    method: 'DELETE',
    url: '/',
    headers: { 'Content-Type': 'application/json' },
    body: null
  }, (err, response) => {
    t.error(err)
    t.equal(response.statusCode, 200)
    t.same(response.payload.toString(), '')
  })
})
Repository: fastify/fastify
File: test/encapsulated-error-handler.test.js (JavaScript)

Code before:
'use strict'

const { test } = require('tap')
const Fastify = require('..')

// Because of how error handlers wrap things, following the control flow can be tricky
// In this test file numbered comments indicate the order statements are expected to execute

test('encapsulates an asynchronous error handler', async t => {
  t.plan(4)

  const fastify = Fastify()
  fastify.register(async function (fastify) {
    fastify.setErrorHandler(async function a (err) {
      // 3. the inner error handler catches the error, and throws a new error
      t.equal(err.message, 'from_endpoint')
      throw new Error('from_inner')
    })
    fastify.get('/encapsulated', async () => {
      // 2. the endpoint throws an error
      throw new Error('from_endpoint')
    })
  })

  fastify.setErrorHandler(async function b (err) {
    // 4. the outer error handler catches the error thrown by the inner error handler
    t.equal(err.message, 'from_inner')
    // 5. the outer error handler throws a new error
    throw new Error('from_outer')
  })

  // 1. the endpoint is called
  const res = await fastify.inject('/encapsulated')
  // 6. the default error handler returns the error from the outer error handler
  t.equal(res.json().message, 'from_outer')

Review comment (suggestion):
```suggestion
  t.plan(3)
```

Why `t.plan` is `4` but not `3`?
I can see three `t.equal` only and none of the code path run twice.

Code after:
'use strict'

const { test } = require('tap')
const Fastify = require('..')

// Because of how error handlers wrap things, following the control flow can be tricky
// In this test file numbered comments indicate the order statements are expected to execute

test('encapsulates an asynchronous error handler', async t => {
  t.plan(3)

  const fastify = Fastify()
  fastify.register(async function (fastify) {
    fastify.setErrorHandler(async function a (err) {
      // 3. the inner error handler catches the error, and throws a new error
      t.equal(err.message, 'from_endpoint')
      throw new Error('from_inner')
    })
    fastify.get('/encapsulated', async () => {
      // 2. the endpoint throws an error
      throw new Error('from_endpoint')
    })
  })

  fastify.setErrorHandler(async function b (err) {
    // 4. the outer error handler catches the error thrown by the inner error handler
    t.equal(err.message, 'from_inner')
    // 5. the outer error handler throws a new error
    throw new Error('from_outer')
  })

  // 1. the endpoint is called
  const res = await fastify.inject('/encapsulated')
  // 6. the default error handler returns the error from the outer error handler
  t.equal(res.json().message, 'from_outer')
Repository: fastify/fastify
File: types/type-provider.d.ts (TypeScript)

Code before:
// -----------------------------------------------------------------------------------------------
// FastifyReplyReturnType
// -----------------------------------------------------------------------------------------------

// The target reply return type. This type is inferenced on fastify 'routes' via generic argument assignment
export type ResolveFastifyReplyReturnType<
  TypeProvider extends FastifyTypeProvider,
  SchemaCompiler extends FastifySchema,
  RouteGeneric extends RouteGenericInterface,
> = ResolveFastifyReplyType<
TypeProvider,
SchemaCompiler,
RouteGeneric
> extends infer Return ?
  (Return | void | Promise<Return | void>)
// review: support both async and sync return types
// (Promise<Return> | Return | Promise<void> | void)
  : unknown

/**
 * This branded type is needed to please `typescript-eslint`.
 *
 * Please refer to the following Github issue for more info:
 * https://github.com/fastify/fastify/issues/5498
 */
export type SafePromiseLike<T> = PromiseLike<T> & { __brand: 'SafePromiseLike' }


Review comment (bug):
I want to ensure we didn't use the wrong property.
It is a bit confuse to me `__brand` is not showing in the document, and `redux-toolkit` is using `__linterBrands`

https://github.com/fastify/fastify/issues/5498#issuecomment-2154566979

Code after:
// FastifyReplyReturnType
// -----------------------------------------------------------------------------------------------

// The target reply return type. This type is inferenced on fastify 'routes' via generic argument assignment
export type ResolveFastifyReplyReturnType<
  TypeProvider extends FastifyTypeProvider,
  SchemaCompiler extends FastifySchema,
  RouteGeneric extends RouteGenericInterface
> = ResolveFastifyReplyType<
TypeProvider,
SchemaCompiler,
RouteGeneric
> extends infer Return ?
    (Return | void | Promise<Return | void>)
// review: support both async and sync return types
// (Promise<Return> | Return | Promise<void> | void)
  : unknown

/**
 * This branded type is needed to indicate APIs that return Promise-likes which can
 * safely "float" (not have rejections handled by calling code).
 *
 * Please refer to the following Github issue for more info:
 * https://github.com/fastify/fastify/issues/5498
 */
export type SafePromiseLike<T> = PromiseLike<T> & { __linterBrands: 'SafePromiseLike' }
Repository: fastify/fastify
File: test/has-route.test.js (JavaScript)

Code before:

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/',
      constraints: { version: '1.2.0' }
    }), true)

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/',
      constraints: { version: '1.3.0' }
    }), false)
  })

  test('hasRoute - parametric route regexp with constraints', t => {
    t.plan(1)
    // parametric with regexp
    fastify.get('/example/:file(^\\d+).png', function (request, reply) { })

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/example/:file(^\\d+).png'
    }), true)
  })

  test('hasRoute - with normalize method', t => {
    t.plan(1)
    fastify.route({
      method: 'GET',
      url: '/equal',
      handler: function (req, reply) {
        reply.send({ hello: 'world' })
      }
    })

    t.equal(fastify.hasRoute({
      method: 'get',
      url: '/equal'
    }), true)
  })
})


Review comment (suggestion):
```suggestion
  test('hasRoute - finds a route even if method is not uppercased', t => {
```

Code after:

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/',
      constraints: { version: '1.2.0' }
    }), true)

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/',
      constraints: { version: '1.3.0' }
    }), false)
  })

  test('hasRoute - parametric route regexp with constraints', t => {
    t.plan(1)
    // parametric with regexp
    fastify.get('/example/:file(^\\d+).png', function (request, reply) { })

    t.equal(fastify.hasRoute({
      method: 'GET',
      url: '/example/:file(^\\d+).png'
    }), true)
  })

  test('hasRoute - finds a route even if method is not uppercased', t => {
    t.plan(1)
    fastify.route({
      method: 'GET',
      url: '/equal',
      handler: function (req, reply) {
        reply.send({ hello: 'world' })
      }
    })

    t.equal(fastify.hasRoute({
      method: 'get',
      url: '/equal'
    }), true)
  })
})
Repository: fastify/fastify
File: eslint.config.js (JavaScript)

Code before:
'use strict'

const neo = require('neostandard')

module.exports = [
  ...neo({
    ts: true,
    ignores: [
      'lib/configValidator.js',
      'lib/error-serializer.js',
      'test/same-shape.test.js',
    ],
  }),
  {
    ignores: [
      'test/same-shape.test.js',
    ],
  },
]


Review comment (suggestion):
@mcollina This restores comma-dangle to what it was in `standard@17`. If the change in neostandard is controversial, then we should revert back to what `standard@17` had, to honor the _descriptive, not prescriptive_ mantra (https://github.com/neostandard/neostandard/issues/7)

A third option would be to simply relax `neostandard` itself so that it accepts both trailing and non-trailing and leave it out of `neostandard` to enforce any specific part

```suggestion
  {
    ignores: [
      'lib/configValidator.js',
      'lib/error-serializer.js',
      'test/same-shape.test.js',
    ],
    rules: {
      '@stylistic/comma-dangle': ['error', {
        arrays: 'never',
        objects: 'never',
        imports: 'never',
        exports: 'never',
        functions: 'never',
      }],
    }
  },
```

Code after:
'use strict'

const neo = require('neostandard')

module.exports = [
  {
    ignores: [
      'lib/configValidator.js',
      'lib/error-serializer.js',
      'test/same-shape.test.js'
    ]
  },
  ...neo({
    ts: true
  }),
  {
    rules: {
      '@stylistic/comma-dangle': ['error', {
        arrays: 'never',
        objects: 'never',
        imports: 'never',
        exports: 'never',
        functions: 'never'
      }]
    }
  }
]
Repository: fastify/fastify
File: test/throw.test.js (JavaScript)

Code before:
const { test } = require('tap')
const Fastify = require('..')
const {
  FST_ERR_DUPLICATED_ROUTE
} = require('../lib/errors')

test('Fastify should throw on wrong options', t => {
  t.plan(2)
  try {
    Fastify('lol')
    t.fail()
  } catch (e) {
    t.equal(e.message, 'Options must be an object')
    t.pass()
  }
})

test('Fastify should throw on multiple assignment to the same route', t => {
  t.plan(1)
  const fastify = Fastify()

  fastify.get('/', () => {})

  t.throws(() => {
    fastify.get('/', () => {})
  }, new FST_ERR_DUPLICATED_ROUTE('GET', '/'))
})

test('Fastify should throw for an invalid schema, printing the error route - headers', t => {
  t.plan(2)

  const badSchema = {
    type: 'object',
    properties: {
      bad: {
        type: 'bad-type'
      }
    }
  }

  const fastify = Fastify()
  fastify.get('/', { schema: { headers: badSchema } }, () => {})
  fastify.get('/not-loaded', { schema: { headers: badSchema } }, () => {})

  fastify.ready(err => {
    t.equal(err.code, 'FST_ERR_SCH_VALIDATION_BUILD')
    t.match(err.message, /Failed building the validation schema for GET: \//)
  })
})

test('Fastify should throw for an invalid schema, printing the error route - body', t => {

Review comment (suggestion):
Instead of matching the error.
Check for `error.code` property, `message` often changed but not consider as breaking change.

Code after:
const { test } = require('tap')
const Fastify = require('..')

test('Fastify should throw on wrong options', t => {
  t.plan(2)
  try {
    Fastify('lol')
    t.fail()
  } catch (e) {
    t.equal(e.message, 'Options must be an object')
    t.pass()
  }
})

test('Fastify should throw on multiple assignment to the same route', t => {
  t.plan(1)
  const fastify = Fastify()

  fastify.get('/', () => {})

  try {
    fastify.get('/', () => {})
    t.fail('Should throw fastify duplicated route declaration')
  } catch (error) {
    t.equal(error.code, 'FST_ERR_DUPLICATED_ROUTE')
  }
})

test('Fastify should throw for an invalid schema, printing the error route - headers', t => {
  t.plan(2)

  const badSchema = {
    type: 'object',
    properties: {
      bad: {
        type: 'bad-type'
      }
    }
  }

  const fastify = Fastify()
  fastify.get('/', { schema: { headers: badSchema } }, () => {})
  fastify.get('/not-loaded', { schema: { headers: badSchema } }, () => {})

  fastify.ready(err => {
    t.equal(err.code, 'FST_ERR_SCH_VALIDATION_BUILD')
    t.match(err.message, /Failed building the validation schema for GET: \//)
  })
})

test('Fastify should throw for an invalid schema, printing the error route - body', t => {