ronantakizawa/codereview-bench huggingface dataset:
Repository: chartjs/Chart.js
File: src/controllers/controller.doughnut.js (JavaScript)

Code before:
    numbers: {
      type: 'number',
      properties: ['circumference', 'endAngle', 'innerRadius', 'outerRadius', 'startAngle', 'x', 'y', 'offset', 'borderWidth', 'spacing']
    },
  },
  // The percentage of the chart that we cut out of the middle.
  cutout: '50%',

  // The rotation of the chart, where the first data arc begins.
  rotation: 0,

  // The total circumference of the chart.
  circumference: 360,

  // The outr radius of the chart
  radius: '100%',

  // Spacing between arcs
  spacing: 0,

  indexAxis: 'r',

  _scriptable: (name) => name !== 'spacing',
  _indexable: (name) => name !== 'spacing',
};

/**
 * @type {any}
 */
DoughnutController.overrides = {
  aspectRatio: 1,

  // Need to override these to give a nice default
  plugins: {
    legend: {
      labels: {
        generateLabels(chart) {
          const data = chart.data;
          if (data.labels.length && data.datasets.length) {
            return data.labels.map((label, i) => {
              const meta = chart.getDatasetMeta(0);
              const style = meta.controller.getStyle(i);

              return {
                text: label,
                fillStyle: style.backgroundColor,
                strokeStyle: style.borderColor,
                lineWidth: style.borderWidth,
                hidden: !chart.getDataVisibility(i),

                // Extra data used for toggling the correct item

Review comment (suggestion):
These should be in `descriptors`, so they are preserved even when user sets `Chart.defaults.doughnut = {...}`

Code after:
    numbers: {
      type: 'number',
      properties: ['circumference', 'endAngle', 'innerRadius', 'outerRadius', 'startAngle', 'x', 'y', 'offset', 'borderWidth', 'spacing']
    },
  },
  // The percentage of the chart that we cut out of the middle.
  cutout: '50%',

  // The rotation of the chart, where the first data arc begins.
  rotation: 0,

  // The total circumference of the chart.
  circumference: 360,

  // The outr radius of the chart
  radius: '100%',

  // Spacing between arcs
  spacing: 0,

  indexAxis: 'r',
};

DoughnutController.descriptors = {
  _scriptable: (name) => name !== 'spacing',
  _indexable: (name) => name !== 'spacing',
};

/**
 * @type {any}
 */
DoughnutController.overrides = {
  aspectRatio: 1,

  // Need to override these to give a nice default
  plugins: {
    legend: {
      labels: {
        generateLabels(chart) {
          const data = chart.data;
          if (data.labels.length && data.datasets.length) {
            return data.labels.map((label, i) => {
              const meta = chart.getDatasetMeta(0);
              const style = meta.controller.getStyle(i);

              return {
                text: label,
                fillStyle: style.backgroundColor,
                strokeStyle: style.borderColor,
                lineWidth: style.borderWidth,
                hidden: !chart.getDataVisibility(i),
Repository: chartjs/Chart.js
File: src/controllers/controller.bar.js (JavaScript)

Code before:
import DatasetController from '../core/core.datasetController';
import {
  _arrayUnique, isArray, isNullOrUndef,
  valueOrDefault, resolveObjectKey, sign, defined
} from '../helpers';

function getAllScaleValues(meta) {
  let scale = meta.iScale;
  if (!scale._cache.$bar) {
    const visibleMetas = scale.getMatchingVisibleMetas(meta._type);
    let values = [];

    for (let i = 0, ilen = visibleMetas.length; i < ilen; i++) {
      values = values.concat(visibleMetas[i].controller.getAllParsedValues(scale));
    }
    scale._cache.$bar = _arrayUnique(values.sort((a, b) => a - b));
  }
  return scale._cache.$bar;
}

/**
 * Computes the "optimal" sample size to maintain bars equally sized while preventing overlap.
 * @private
 */
function computeMinSampleSize(meta) {
  const scale = meta.iScale;
  const values = getAllScaleValues(meta);
  let min = scale._length;
  let i, ilen, curr, prev;
  const updateMinAndPrev = () => {
    if (curr === 32767 || curr === -32768) {
      // Ingnore truncated pixels
      return;
    }
    if (defined(prev)) {

Review comment (suggestion):
should be `meta.type` (or `datasetController._type`)

I'd keep the `scale` parameter, as the function is called `getAllScaleValues` and add `type` as 2nd argument.

Code after:
import DatasetController from '../core/core.datasetController';
import {
  _arrayUnique, isArray, isNullOrUndef,
  valueOrDefault, resolveObjectKey, sign, defined
} from '../helpers';

function getAllScaleValues(scale, type) {
  if (!scale._cache.$bar) {
    const visibleMetas = scale.getMatchingVisibleMetas(type);
    let values = [];

    for (let i = 0, ilen = visibleMetas.length; i < ilen; i++) {
      values = values.concat(visibleMetas[i].controller.getAllParsedValues(scale));
    }
    scale._cache.$bar = _arrayUnique(values.sort((a, b) => a - b));
  }
  return scale._cache.$bar;
}

/**
 * Computes the "optimal" sample size to maintain bars equally sized while preventing overlap.
 * @private
 */
function computeMinSampleSize(meta) {
  const scale = meta.iScale;
  const values = getAllScaleValues(scale, meta.type);
  let min = scale._length;
  let i, ilen, curr, prev;
  const updateMinAndPrev = () => {
    if (curr === 32767 || curr === -32768) {
      // Ingnore truncated pixels
      return;
    }
    if (defined(prev)) {
      // curr - prev === 0 is ignored
Repository: chartjs/Chart.js
File: test/specs/core.controller.tests.js (JavaScript)

Code before:
      this.chart.update();
      metasets = this.chart._metasets;
      expect(metasets[0].label).toEqual('4');
      expect(metasets[1].label).toEqual('3');
      expect(metasets[2].label).toEqual('2');
      expect(metasets[3].label).toEqual('new');
    });
  });

  describe('_destroyDatasetMeta', function() {
    beforeEach(function() {
      this.chart = acquireChart({
        type: 'line',
        data: {
          datasets: [
            {label: '1', order: 2},
            {label: '2', order: 1},
            {label: '3', order: 4},
            {label: '4', order: 3},
          ]
        }
      });
    });
    it('cleans up metasets when the chart is destroyed', function() {
      this.chart.destroy();
      expect(this.chart._metasets.length).toHaveSize(0);
    });
  });

  describe('data visibility', function() {
    it('should hide a dataset', function() {
      var chart = acquireChart({
        type: 'line',
        data: {
          datasets: [{
            data: [0, 1, 2]
          }],
          labels: ['a', 'b', 'c']
        }
      });

      chart.setDatasetVisibility(0, false);

      var meta = chart.getDatasetMeta(0);
      expect(meta.hidden).toBe(true);
    });

    it('should toggle data visibility by index', function() {
      var chart = acquireChart({
        type: 'pie',
        data: {

Review comment (suggestion):
```suggestion
      expect(this.chart._metasets).toHaveSize(0);
```


> toHaveSize(expected)
>expect the actual size to be equal to the expected, using array-like length or object keys size.
>
>array = [1,2];
>expect(array).toHaveSize(2);

https://jasmine.github.io/api/edge/matchers.html

Code after:
      this.chart.update();
      metasets = this.chart._metasets;
      expect(metasets[0].label).toEqual('4');
      expect(metasets[1].label).toEqual('3');
      expect(metasets[2].label).toEqual('2');
      expect(metasets[3].label).toEqual('new');
    });
  });

  describe('_destroyDatasetMeta', function() {
    beforeEach(function() {
      this.chart = acquireChart({
        type: 'line',
        data: {
          datasets: [
            {label: '1', order: 2},
            {label: '2', order: 1},
            {label: '3', order: 4},
            {label: '4', order: 3},
          ]
        }
      });
    });
    it('cleans up metasets when the chart is destroyed', function() {
      this.chart.destroy();
      expect(this.chart._metasets).toEqual([undefined, undefined, undefined, undefined]);
    });
  });

  describe('data visibility', function() {
    it('should hide a dataset', function() {
      var chart = acquireChart({
        type: 'line',
        data: {
          datasets: [{
            data: [0, 1, 2]
          }],
          labels: ['a', 'b', 'c']
        }
      });

      chart.setDatasetVisibility(0, false);

      var meta = chart.getDatasetMeta(0);
      expect(meta.hidden).toBe(true);
    });

    it('should toggle data visibility by index', function() {
      var chart = acquireChart({
        type: 'pie',
        data: {
Repository: chartjs/Chart.js
File: src/core/core.controller.js (JavaScript)

Code before:
    let i, ilen;
    this.stop();
    animator.remove(this);

    for (i = 0, ilen = this.data.datasets.length; i < ilen; ++i) {
      this._destroyDatasetMeta(i);
    }
  }

  destroy() {
    this.notifyPlugins('beforeDestroy');
    const {canvas, ctx} = this;

    this._stop();
    this.config.clearCache();

    if (canvas) {
      this.unbindEvents();
      clearCanvas(canvas, ctx);
      this.platform.releaseContext(ctx);
      this.canvas = null;
      this.ctx = null;
    }

    this.notifyPlugins('destroy');
    this.notifyPlugins('afterDestroy');

    delete instances[this.id];
  }

  toBase64Image(...args) {
    return this.canvas.toDataURL(...args);
  }

  /**
	 * @private
	 */
  bindEvents() {
    this.bindUserEvents();
    if (this.options.responsive) {
      this.bindResponsiveEvents();
    } else {
      this.attached = true;
    }
  }

  /**
   * @private
   */
  bindUserEvents() {
    const listeners = this._listeners;

Review comment (refactor):
I'd move this after removing from instances. But keep the `destroy` hook as is until its removed.
Could add a comment like
// TODO v4: remove destroy hook

Code after:
    let i, ilen;
    this.stop();
    animator.remove(this);

    for (i = 0, ilen = this.data.datasets.length; i < ilen; ++i) {
      this._destroyDatasetMeta(i);
    }
  }

  destroy() {
    this.notifyPlugins('beforeDestroy');
    const {canvas, ctx} = this;

    this._stop();
    this.config.clearCache();

    if (canvas) {
      this.unbindEvents();
      clearCanvas(canvas, ctx);
      this.platform.releaseContext(ctx);
      this.canvas = null;
      this.ctx = null;
    }

    // TODO V4: delete destroy hook and reference to it in plugin flowchart
    this.notifyPlugins('destroy');

    delete instances[this.id];

    this.notifyPlugins('afterDestroy');
  }

  toBase64Image(...args) {
    return this.canvas.toDataURL(...args);
  }

  /**
	 * @private
	 */
  bindEvents() {
    this.bindUserEvents();
    if (this.options.responsive) {
      this.bindResponsiveEvents();
    } else {
      this.attached = true;
    }
  }

  /**
   * @private
   */
Repository: chartjs/Chart.js
File: src/controllers/controller.polarArea.js (JavaScript)

Code before:
    // Need to override these to give a nice default
    tooltip: {
      callbacks: {
        title() {
          return '';
        },
        label(context) {
          return context.chart.data.labels[context.dataIndex] + ': ' + context.formattedValue;
        }
      }
    }
  },

  scales: {
    r: {
      type: 'radialLinear',
      angleLines: {
        display: false
      },
      beginAtZero: true,
      grid: {
        circular: true
      },
      pointLabels: {
        display: false,
        polarChartLabel: true
      },
      startAngle: 0
    }
  }
};


Review comment (suggestion):
centerPointLabels or centerLabels would be a more logical name to me since its implemented on scale level the radar chart could also use it (if wanted for some reason)

Also it cant be true by default. This will make it a breaking change 

Code after:
    // Need to override these to give a nice default
    tooltip: {
      callbacks: {
        title() {
          return '';
        },
        label(context) {
          return context.chart.data.labels[context.dataIndex] + ': ' + context.formattedValue;
        }
      }
    }
  },

  scales: {
    r: {
      type: 'radialLinear',
      angleLines: {
        display: false
      },
      beginAtZero: true,
      grid: {
        circular: true
      },
      pointLabels: {
        display: false
      },
      startAngle: 0
    }
  }
};
Repository: chartjs/Chart.js
File: test/specs/helpers.dom.tests.js (JavaScript)

Code before:
      expect(dataX).not.toEqual(NaN);
      expect(dataY).not.toEqual(NaN);
    });

    it('Should give consistent results for native and chart events', async function() {
      let chartPosition = null;
      const chart = window.acquireChart(
        {
          type: 'bar',
          data: {
            datasets: [{
              data: [{x: 'first', y: 10}, {x: 'second', y: 5}, {x: 'third', y: 15}]
            }]
          },
          options: {
            onHover: (chartEvent) => {
              chartPosition = Chart.helpers.getRelativePosition(chartEvent, chart);
            }
          }
        });

      const point = chart.getDatasetMeta(0).data[1];
      const nativeEvent = await jasmine.triggerMouseEvent(chart, 'mousemove', point);
      const nativePosition = Chart.helpers.getRelativePosition(nativeEvent, chart);

      expect(nativePosition).toEqual(chartPosition);
    });
  });
});


Review comment (suggestion):
can you add one more expect here so that we know `chartPosition !== null`. Just want to confirm the `onHover` callback actually ran in case the `nativeEvent` is also somehow null

Code after:
      expect(dataX).not.toEqual(NaN);
      expect(dataY).not.toEqual(NaN);
    });

    it('Should give consistent results for native and chart events', async function() {
      let chartPosition = null;
      const chart = window.acquireChart(
        {
          type: 'bar',
          data: {
            datasets: [{
              data: [{x: 'first', y: 10}, {x: 'second', y: 5}, {x: 'third', y: 15}]
            }]
          },
          options: {
            onHover: (chartEvent) => {
              chartPosition = Chart.helpers.getRelativePosition(chartEvent, chart);
            }
          }
        });

      const point = chart.getDatasetMeta(0).data[1];
      const nativeEvent = await jasmine.triggerMouseEvent(chart, 'mousemove', point);
      const nativePosition = Chart.helpers.getRelativePosition(nativeEvent, chart);

      expect(chartPosition).not.toBeNull();
      expect(nativePosition).toEqual({x: chartPosition.x, y: chartPosition.y});
    });
  });
});
Repository: chartjs/Chart.js
File: src/controllers/controller.polarArea.js (JavaScript)

Code before:
  }

  getLabelAndValue(index) {
    const meta = this._cachedMeta;
    const chart = this.chart;
    const labels = chart.data.labels || [];
    const value = formatNumber(meta._parsed[index].r, chart.options.locale);

    return {
      label: labels[index] || '',
      value,
    };
  }

  parseObjectData(meta, data, start, count) {
    const {iScale} = meta;
    const {key = 'key'} = this._parsing;
    const parsed = new Array(count);
    let i, ilen, index, item;

    for (i = 0, ilen = count; i < ilen; ++i) {
      index = i + start;
      item = data[index];
      parsed[i] = {
        r: iScale.parse(resolveObjectKey(item, key), index)
      };
    }

    return parsed;
  }

  update(mode) {
    const arcs = this._cachedMeta.data;

    this._updateRadius();
    this.updateElements(arcs, 0, arcs.length, mode);
  }

  /**
	 * @private
	 */
  _updateRadius() {
    const chart = this.chart;
    const chartArea = chart.chartArea;
    const opts = chart.options;
    const minSize = Math.min(chartArea.right - chartArea.left, chartArea.bottom - chartArea.top);

    const outerRadius = Math.max(minSize / 2, 0);
    const innerRadius = Math.max(opts.cutoutPercentage ? (outerRadius / 100) * (opts.cutoutPercentage) : 1, 0);
    const radiusLength = (outerRadius - innerRadius) / chart.getVisibleDatasetCount();


Review comment (suggestion):
```suggestion
      parsed[i] = iScale.parse(resolveObjectKey(item, key), index);
```

Code after:
  }

  getLabelAndValue(index) {
    const meta = this._cachedMeta;
    const chart = this.chart;
    const labels = chart.data.labels || [];
    const value = formatNumber(meta._parsed[index].r, chart.options.locale);

    return {
      label: labels[index] || '',
      value,
    };
  }

  parseObjectData(meta, data, start, count) {
    return _parseObjectDataRadialScale.bind(this)(meta, data, start, count);
  }

  update(mode) {
    const arcs = this._cachedMeta.data;

    this._updateRadius();
    this.updateElements(arcs, 0, arcs.length, mode);
  }

  /**
	 * @private
	 */
  _updateRadius() {
    const chart = this.chart;
    const chartArea = chart.chartArea;
    const opts = chart.options;
    const minSize = Math.min(chartArea.right - chartArea.left, chartArea.bottom - chartArea.top);

    const outerRadius = Math.max(minSize / 2, 0);
    const innerRadius = Math.max(opts.cutoutPercentage ? (outerRadius / 100) * (opts.cutoutPercentage) : 1, 0);
    const radiusLength = (outerRadius - innerRadius) / chart.getVisibleDatasetCount();

    this.outerRadius = outerRadius - (radiusLength * this.index);
    this.innerRadius = this.outerRadius - radiusLength;
  }

  updateElements(arcs, start, count, mode) {
    const reset = mode === 'reset';
    const chart = this.chart;
    const opts = chart.options;
    const animationOpts = opts.animation;
    const scale = this._cachedMeta.rScale;
    const centerX = scale.xCenter;
    const centerY = scale.yCenter;
    const datasetStartAngle = scale.getIndexAngle(0) - 0.5 * PI;
Repository: chartjs/Chart.js
File: src/controllers/controller.polarArea.js (JavaScript)

Code before:
import {toRadians, PI, formatNumber, resolveObjectKey} from '../helpers/index';

export default class PolarAreaController extends DatasetController {

  constructor(chart, datasetIndex) {
    super(chart, datasetIndex);

    this.innerRadius = undefined;
    this.outerRadius = undefined;
  }

  getLabelAndValue(index) {
    const meta = this._cachedMeta;
    const chart = this.chart;
    const labels = chart.data.labels || [];
    const value = formatNumber(meta._parsed[index].r, chart.options.locale);

    return {
      label: labels[index] || '',
      value,
    };
  }

  parseObjectData(meta, data, start, count) {
    const {iScale} = meta;
    const {key = 'key'} = this._parsing;
    const parsed = new Array(count);
    let i, ilen, index, item;

    for (i = 0, ilen = count; i < ilen; ++i) {
      index = i + start;
      item = data[index];
      parsed[i] = {
        r: iScale.parse(resolveObjectKey(item, key), index)
      };
    }

    return parsed;
  }

  update(mode) {
    const arcs = this._cachedMeta.data;

    this._updateRadius();
    this.updateElements(arcs, 0, arcs.length, mode);
  }

  /**
	 * @private
	 */
  _updateRadius() {

Review comment (suggestion):
I think this "default" will be used when `parsing = true`. Should it be `'r'` or `'value'` rather than `'key'`?
Should probably add a test for that too.

Code after:
import {toRadians, PI, formatNumber, _parseObjectDataRadialScale} from '../helpers/index';

export default class PolarAreaController extends DatasetController {

  constructor(chart, datasetIndex) {
    super(chart, datasetIndex);

    this.innerRadius = undefined;
    this.outerRadius = undefined;
  }

  getLabelAndValue(index) {
    const meta = this._cachedMeta;
    const chart = this.chart;
    const labels = chart.data.labels || [];
    const value = formatNumber(meta._parsed[index].r, chart.options.locale);

    return {
      label: labels[index] || '',
      value,
    };
  }

  parseObjectData(meta, data, start, count) {
    return _parseObjectDataRadialScale.bind(this)(meta, data, start, count);
  }

  update(mode) {
    const arcs = this._cachedMeta.data;

    this._updateRadius();
    this.updateElements(arcs, 0, arcs.length, mode);
  }

  /**
	 * @private
	 */
  _updateRadius() {
    const chart = this.chart;
    const chartArea = chart.chartArea;
    const opts = chart.options;
    const minSize = Math.min(chartArea.right - chartArea.left, chartArea.bottom - chartArea.top);

    const outerRadius = Math.max(minSize / 2, 0);
    const innerRadius = Math.max(opts.cutoutPercentage ? (outerRadius / 100) * (opts.cutoutPercentage) : 1, 0);
    const radiusLength = (outerRadius - innerRadius) / chart.getVisibleDatasetCount();

    this.outerRadius = outerRadius - (radiusLength * this.index);
    this.innerRadius = this.outerRadius - radiusLength;
  }
Repository: chartjs/Chart.js
File: types/helpers/helpers.color.d.ts (TypeScript)

Code before:
import { AnyObject } from '../basic';

export function color(value: CanvasGradient): CanvasGradient;
export function color(value: CanvasPattern): CanvasPattern;
export function color(
  value:
  | string
  | { r: number; g: number; b: number; a: number }
  | [number, number, number]
  | [number, number, number, number]
): ColorModel;

export function isPatternOrGradient(value: AnyObject): boolean;

export interface ColorModel {
  rgbString(): string;
  hexString(): string;
  hslString(): string;
  rgb: { r: number; g: number; b: number; a: number };
  valid: boolean;
  mix(color: ColorModel, weight: number): this;
  clone(): ColorModel;
  alpha(a: number): ColorModel;
  clearer(ration: number): ColorModel;
  greyscale(): ColorModel;
  opaquer(ratio: number): ColorModel;
  negate(): ColorModel;
  lighten(ratio: number): ColorModel;
  darken(ratio: number): ColorModel;
  saturate(ratio: number): ColorModel;
  desaturate(ratio: number): ColorModel;
  rotate(deg: number): this;
}

export function getHoverColor(value: CanvasGradient): CanvasGradient;
export function getHoverColor(value: CanvasPattern): CanvasPattern;
export function getHoverColor(value: string): string;


Review comment (suggestion):
I think `Color` would suit the value better (string|CanvasGradient|CanvasPattern)

Code after:
import { AnyObject } from '../basic';

export function color(value: CanvasGradient): CanvasGradient;
export function color(value: CanvasPattern): CanvasPattern;
export function color(
  value:
  | string
  | { r: number; g: number; b: number; a: number }
  | [number, number, number]
  | [number, number, number, number]
): ColorModel;

export function isPatternOrGradient(value: string | AnyObject): boolean;

export interface ColorModel {
  rgbString(): string;
  hexString(): string;
  hslString(): string;
  rgb: { r: number; g: number; b: number; a: number };
  valid: boolean;
  mix(color: ColorModel, weight: number): this;
  clone(): ColorModel;
  alpha(a: number): ColorModel;
  clearer(ration: number): ColorModel;
  greyscale(): ColorModel;
  opaquer(ratio: number): ColorModel;
  negate(): ColorModel;
  lighten(ratio: number): ColorModel;
  darken(ratio: number): ColorModel;
  saturate(ratio: number): ColorModel;
  desaturate(ratio: number): ColorModel;
  rotate(deg: number): this;
}

export function getHoverColor(value: CanvasGradient): CanvasGradient;
export function getHoverColor(value: CanvasPattern): CanvasPattern;
export function getHoverColor(value: string): string;