ronantakizawa/codereview-bench huggingface dataset:
Repository: kestra-io/kestra
File: core/src/main/java/io/kestra/core/exceptions/KilledException.java (Java)

Code before:
package io.kestra.core.exceptions;

import java.io.Serial;

/**
 * Exception thrown when a task runner is killed during execution.
 */
public class KilledException extends KestraRuntimeException {
    private static final String DEFAULT_MESSAGE = "Task runner was killed during execution.";
    private static final String PHASE_MESSAGE = "Task runner was killed during %s.";

    /**
     * Creates a new {@link KilledException} with a default message.
     */
    public KilledException() {
        super(DEFAULT_MESSAGE);
    }

    /**
     * Creates a new {@link KilledException} with a message describing
     * the execution phase during which the kill occurred.
     *
     * @param phase a short description of the execution phase
     *              (e.g. "image pull", "container start").
     */
    public KilledException(String phase) {
        super(PHASE_MESSAGE.formatted(phase));
    }
}


Review comment (suggestion):
Classic implementation is when you have a message, tu be able to se the full message not some part.
I would prefer that when you take a message in the constructor parameter, it would be the full message not only a part of it

Code after:
package io.kestra.core.exceptions;

import java.io.Serial;

/**
 * Exception thrown when a task runner is killed during execution.
 */
public class KilledException extends KestraRuntimeException {
    private static final String DEFAULT_MESSAGE = "Execution was killed.";

    /**
     * Creates a new {@link KilledException} with a default message.
     */
    public KilledException() {
        super(DEFAULT_MESSAGE);
    }

    /**
     * Creates a new {@link KilledException} with a message describing
     * the execution phase during which the kill occurred.
     *
     * @param message the error message.
     */
    public KilledException(String message) {
        super(message);
    }
}
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:
   * @api public
   */

  get: function(name){
    return this.header[name.toLowerCase()];
  },

  /**
   * Set header `field` to `val`, or pass
   * an object of header fields.
   *
   * Examples:
   *
   *    this.set('Foo', ['bar', 'baz']);
   *    this.set('Accept', 'application/json');
   *    this.set({ Accept: 'text/plain', 'X-API-Key': 'tobi' });
   *
   * @param {String|Object|Array} field
   * @param {String} val
   * @api public
   */

  set: function(field, val){
    if (2 == arguments.length) {
      if (Array.isArray(val)) val = val.map(String);
      else val = String(val);
      this.res.setHeader(field, val);
    } else {
      for (var key in field) {
        this.set(key, field[key]);
      }
    }
  },

  /**
   * Append `val` to header `field`.
   *
   * @param {String} field
   * @param {String} val
   * @api public
   */

  append: function(field, val){
    field = field.toLowerCase();
    var header = this.header;
    var list = header[field];

    // not set
    if (!list) return this.set(field, val);

    // append

Review comment (suggestion):
if this doesn't unwrap quotes or anything we should probably remove it so we don't encourage adding getters for every field out there


Code after:
  /**
   * Set the ETag of a response.
   * This will normalize the quotes if necessary.
   *
   *     this.response.etag = 'md5hashsum';
   *     this.response.etag = '"md5hashsum"';
   *     this.response.etag = 'W/"123456789"';
   *
   * @param {String} etag
   * @api public
   */

  set etag(val) {
    if (!/^(W\/)?".*"$/.test(val)) val = '"' + val + '"';
    this.set('ETag', val);
  },

  /**
   * Get the ETag of a response.
   *
   * @return {String}
   * @api public
   */

  get etag() {
    return this.get('etag');
  },

  /**
   * Return the request mime type void of
   * parameters such as "charset".
   *
   * @return {String}
   * @api public
   */

  get type() {
    var type = this.get('Content-Type');
    if (!type) return;
    return type.split(';')[0];
  },

  /**
   * Return response header.
   *
   * Examples:
   *
   *     this.get('Content-Type');
   *     // => "text/plain"
   *
   *     this.get('content-type');
Repository: koajs/koa
File: lib/application.js (JavaScript)

Code before:
  var gen = compose(mw);
  var self = this;

  return function(req, res, next){
    var ctx = self.createContext(req, res);

    co.call(ctx, gen)(next || ctx.onerror);
  }
};

/**
 * Initialize a new context.
 *
 * @api private
 */

app.createContext = function(req, res){
  var context = Object.create(this.context);
  var request = context.request = Object.create(this.request);
  var response = context.response = Object.create(this.response);
  context.app = request.app = response.app = this;
  context.req = request.req = response.req = req;
  context.res = request.res = response.res = res;
  request.ctx = response.ctx = context;
  context.onerror = context.onerror.bind(context);
  context.cookies = new Cookies(req, res);
  context.originalUrl = request.originalUrl = req.url;
  return context;
}

/**
 * Default error handler.
 *
 * @param {Error} err
 * @api private
 */

app.onerror = function(err){
  if (!this.outputErrors) return;
  if (404 == err.status) return;
  console.error(err.stack);
};

/**
 * Response middleware.
 */

function *respond(next){
  this.status = 200;
  if (this.app.poweredBy) this.set('X-Powered-By', 'koa');


Review comment (suggestion):
So I generally don't do instanceof checks because it doesn't work across contexts ex if they use a different version or module for key grip. It'll work for now, but someone might complain about it eventually. That's why I did app.keygrip= and not bother checkjng


Code after:
  var mw = [respond].concat(this.middleware);
  var gen = compose(mw);
  var self = this;

  return function(req, res, next){
    var ctx = self.createContext(req, res);
    co.call(ctx, gen)(next || ctx.onerror);
  }
};

/**
 * Set signed cookie keys.
 * 
 * These are passed to [KeyGrip](https://github.com/jed/keygrip),
 * however you may also pass your own `KeyGrip` instance. For
 * example the following are acceptable:
 *
 *   app.keys = ['im a newer secret', 'i like turtle'];
 *   app.keys = new KeyGrip(['im a newer secret', 'i like turtle'], 'sha256');
 *
 * @param {Array|KeyGrip} keys
 * @api public
 */

app.__defineSetter__('keys', function(keys){
  var ok = Array.isArray(keys) || keys instanceof Keygrip;
  if (!ok) throw new TypeError('app.keys must be an array or Keygrip');
  if (!(keys instanceof Keygrip)) keys = new Keygrip(keys);
  this._keys = keys;
});

/**
 * Get `Keygrip` instance.
 *
 * @return {Keygrip}
 * @api public
 */

app.__defineGetter__('keys', function(){
  return this._keys;
});

/**
 * Initialize a new context.
 *
 * @api private
 */

app.createContext = function(req, res){
  var context = Object.create(this.context);
  var request = context.request = Object.create(this.request);
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:

  get header() {
    // TODO: wtf
    return this.res._headers || {};
  },

  /**
   * Return response status string.
   *
   * @return {String}
   * @api public
   */

  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} val
   * @api public
   */

  set status(val) {
    if ('string' == typeof val) {
      var n = statuses[val.toLowerCase()];
      if (!n) throw new Error(statusError(val));
      val = n;
    }

    this.res.statusCode = val;

    var noContent = 304 == this.status || 204 == this.status;
    if (noContent && this.body) this.body = null;
  },

  /**
   * Get response body.
   *

Review comment (suggestion):
this is a little weird now too, undefined vs the 200 node would normally give you, we could default the 404 here


Code after:

  get header() {
    // TODO: wtf
    return this.res._headers || {};
  },

  /**
   * Return response status string.
   *
   * @return {String}
   * @api public
   */

  get statusString() {
    return http.STATUS_CODES[this.status || 404];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this._status;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} val
   * @api public
   */

  set status(val) {
    if ('string' == typeof val) {
      var n = statuses[val.toLowerCase()];
      if (!n) throw new Error(statusError(val));
      val = n;
    }

    var status = this._status = this.res.statusCode = val;
    var noContent = 304 == status || 204 == status;
    if (noContent && this.body) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:

  get header() {
    // TODO: wtf
    return this.res._headers || {};
  },

  /**
   * Return response status string.
   *
   * @return {String}
   * @api public
   */

  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} val
   * @api public
   */

  set status(val) {
    if ('string' == typeof val) {
      var n = statuses[val.toLowerCase()];
      if (!n) throw new Error(statusError(val));
      val = n;
    }

    this.res.statusCode = val;

    var noContent = 304 == this.status || 204 == this.status;
    if (noContent && this.body) this.body = null;
  },

  /**
   * Get response body.
   *

Review comment (suggestion):
yeah was thinking about defaulting to `404`. one difference is explicitly setting the response as "handled" and "unhandled". so i can set `this.status = 404` and `static` would skip it. 

i think there were other arguments in my head against defaulting to `404`, don't remember what they were.


Code after:

  get header() {
    // TODO: wtf
    return this.res._headers || {};
  },

  /**
   * Return response status string.
   *
   * @return {String}
   * @api public
   */

  get statusString() {
    return http.STATUS_CODES[this.status || 404];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this._status;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} val
   * @api public
   */

  set status(val) {
    if ('string' == typeof val) {
      var n = statuses[val.toLowerCase()];
      if (!n) throw new Error(statusError(val));
      val = n;
    }

    var status = this._status = this.res.statusCode = val;
    var noContent = 304 == status || 204 == status;
    if (noContent && this.body) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:
    var setType = !this.header['content-type'];

    // string
    if ('string' == typeof val) {
      if (setType) this.type = /^\s*</.test(val) ? 'html' : 'text';
      this.length = Buffer.byteLength(val);
      return;
    }

    // buffer
    if (Buffer.isBuffer(val)) {
      if (setType) this.type = 'bin';
      this.length = val.length;
      return;
    }

    // stream
    if ('function' == typeof val.pipe) {
      if (setType) this.type = 'bin';
      return;
    }

    // json
    this.type = 'json';
  },

  /**
   * Set Content-Length field to `n`.
   *
   * @param {Number} n
   * @api public
   */

  set length(n) {
    this.set('Content-Length', n);
  },

  /**
   * Return parsed response Content-Length when present.
   *
   * @return {Number}
   * @api public
   */

  get length() {
    var len = this.header['content-length'];
    var body = this.body;

    if (null == len) {
      if (!body) return;
      if ('string' == typeof body) return Buffer.byteLength(body);

Review comment (suggestion):
probably fine in most cases without this, I don't see people actually registering it themselves they'll just forget or whatever and onerror is semi-private anyway


Code after:
    this.status = this.status || 200;

    // set the content-type only if not yet set
    var setType = !this.header['content-type'];

    // string
    if ('string' == typeof val) {
      if (setType) this.type = /^\s*</.test(val) ? 'html' : 'text';
      this.length = Buffer.byteLength(val);
      return;
    }

    // buffer
    if (Buffer.isBuffer(val)) {
      if (setType) this.type = 'bin';
      this.length = val.length;
      return;
    }

    // stream
    if ('function' == typeof val.pipe) {
      // make sure all intermediate streams will destroy on finished
      onFinished(this, destroy.bind(null, val));

      // make sure all intermediate streams has an listenner on error
      inject(val, this.ctx.onerror);

      if (setType) this.type = 'bin';
      return;
    }

    // json
    this.type = 'json';
  },

  /**
   * Set Content-Length field to `n`.
   *
   * @param {Number} n
   * @api public
   */

  set length(n) {
    this.set('Content-Length', n);
  },

  /**
   * Return parsed response Content-Length when present.
   *
   * @return {Number}
   * @api public
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
   */

  set body(val) {
    this._body = val;

    // no content
    if (null == val) {
      if (!status.empty[this.status]) this.status = 204;
      return;
    }

    // set the status
    if (!this.status) this.status = 200;

    // set the content-type only if not yet set
    var setType = !this.header['content-type'];

    // string
    if ('string' == typeof val) {
      if (setType) this.type = /^\s*</.test(val) ? 'html' : 'text';
      this.length = Buffer.byteLength(val);
      return;
    }

    // buffer
    if (Buffer.isBuffer(val)) {
      if (setType) this.type = 'bin';
      this.length = val.length;
      return;
    }

    // stream
    if ('function' == typeof val.pipe) {
      if (setType) this.type = 'bin';
      return;
    }

    // json

Review comment (suggestion):
hmm I definitely prefer these here since upstream middleware may be acting on these headers, it's best if we remove them right away


Code after:
   *
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
   */

  set body(val) {
    this._body = val;

    // no content
    if (null == val) {
      if (!status.empty[this.status]) this.status = 204;
      this.res.removeHeader('Content-Type');
      this.res.removeHeader('Content-Length');
      this.res.removeHeader('Transfer-Encoding');
      return;
    }

    // set the status
    if (!this.status) this.status = 200;

    // set the content-type only if not yet set
    var setType = !this.header['content-type'];

    // string
    if ('string' == typeof val) {
      if (setType) this.type = /^\s*</.test(val) ? 'html' : 'text';
      this.length = Buffer.byteLength(val);
      return;
    }

    // buffer
    if (Buffer.isBuffer(val)) {
      if (setType) this.type = 'bin';
      this.length = val.length;
      return;
    }

    // stream
    if ('function' == typeof val.pipe) {
      if (setType) this.type = 'bin';
Repository: koajs/koa
File: lib/application.js (JavaScript)

Code before:
app.createContext = function(req, res){
  var context = Object.create(this.context);
  var request = context.request = Object.create(this.request);
  var response = context.response = Object.create(this.response);
  context.app = request.app = response.app = this;
  context.req = request.req = response.req = req;
  context.res = request.res = response.res = res;
  request.ctx = response.ctx = context;
  request.response = response;
  response.request = request;
  context.onerror = context.onerror.bind(context);
  context.originalUrl = request.originalUrl = req.url;
  context.cookies = new Cookies(req, res, this.keys);
  context.accept = request.accept = accepts(req);
  return context;
};

/**
 * Default error handler.
 *
 * @param {Error} err
 * @api private
 */

app.onerror = function(err){
  if (404 == err.status) return;
  if ('test' == this.env) return;
  console.error();
  console.error(err.stack.replace(/^/gm, '  '));
  console.error();
};

/**
 * Response middleware.
 */

function *respond(next) {
  if (this.app.poweredBy) this.set('X-Powered-By', 'koa');

  yield *next;

  // allow bypassing koa
  if (false === this.respond) return;

  var res = this.res;
  if (res.headersSent || !this.writable) return;

  var body = this.body;
  var code = this.status;

  // ignore body

Review comment (bug):
We dont need that line. We can easily expect the function to be called with an value, and if its not we can fix it at the origin, this line will just stop us from finding possible bugs


Code after:
app.createContext = function(req, res){
  var context = Object.create(this.context);
  var request = context.request = Object.create(this.request);
  var response = context.response = Object.create(this.response);
  context.app = request.app = response.app = this;
  context.req = request.req = response.req = req;
  context.res = request.res = response.res = res;
  request.ctx = response.ctx = context;
  request.response = response;
  response.request = request;
  context.onerror = context.onerror.bind(context);
  context.originalUrl = request.originalUrl = req.url;
  context.cookies = new Cookies(req, res, this.keys);
  context.accept = request.accept = accepts(req);
  return context;
};

/**
 * Default error handler.
 *
 * @param {Error} err
 * @api private
 */

app.onerror = function(err){
  if (!err) return;
  assert(err instanceof Error, 'non-error thrown: ' + err);

  if (404 == err.status) return;
  if ('test' == this.env) return;

  var msg = err.stack || err.toString();
  console.error();
  console.error(msg.replace(/^/gm, '  '));
  console.error();
};

/**
 * Response middleware.
 */

function *respond(next) {
  if (this.app.poweredBy) this.set('X-Powered-By', 'koa');

  yield *next;

  // allow bypassing koa
  if (false === this.respond) return;

  var res = this.res;
  if (res.headersSent || !this.writable) return;
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:
   */

  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} code
   * @api public
   */

  set status(code) {
    if(typeof code !== 'number') throw new Error('status code must be an integer');
    if(!http.STATUS_CODES[code]) throw new Error('invalid status code: ' + code);
    this._explicitStatus = true;
    this.res.statusCode = code;
    if (this.body && status.empty[code]) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
   */

  set body(val) {

Review comment (suggestion):
I forget where this status() thing is off the top of my head, but we could probably remove that then as well


Code after:
   * @api public
   */

  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number} code
   * @api public
   */

  set status(code) {
    assert(typeof code === 'number', 'status code must be a number');
    if(!http.STATUS_CODES[code]) throw new Error('invalid status code: ' + code);

    this._explicitStatus = true;
    this.res.statusCode = code;

    if (this.body && status.empty[code]) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
Repository: koajs/koa
File: lib/response.js (JavaScript)

Code before:
  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number|String} code
   * @api public
   */

  set status(code) {
    if(typeof code !== 'number') throw new Error('status code must be an integer');
    if(!http.STATUS_CODES[code]) throw new Error('invalid status code: ' + code);
    this._explicitStatus = true;
    this.res.statusCode = code;
    if (this.body && status.empty[code]) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
   */

  set body(val) {
    var original = this._body;
    this._body = val;

Review comment (bug):
lets just do `assert(typeof code == 'number', 'status code must be a number')`, the assertion following it would fail on things like 'toString' etc


Code after:

  get statusString() {
    return http.STATUS_CODES[this.status];
  },

  /**
   * Get response status code.
   *
   * @return {Number}
   * @api public
   */

  get status() {
    return this.res.statusCode;
  },

  /**
   * Set response status code.
   *
   * @param {Number} code
   * @api public
   */

  set status(code) {
    assert(typeof code === 'number', 'status code must be a number');
    if(!http.STATUS_CODES[code]) throw new Error('invalid status code: ' + code);

    this._explicitStatus = true;
    this.res.statusCode = code;

    if (this.body && status.empty[code]) this.body = null;
  },

  /**
   * Get response body.
   *
   * @return {Mixed}
   * @api public
   */

  get body() {
    return this._body;
  },

  /**
   * Set response body.
   *
   * @param {String|Buffer|Object|Stream} val
   * @api public
   */