ronantakizawa/codereview-bench huggingface dataset:
Repository: crewAIInc/crewAI
File: lib/crewai/tests/telemetry/test_telemetry.py (Python)

Code before:
    for thread in threads:
        thread.join()

    assert all(instance is telemetry1 for instance in instances)


def test_no_signal_handler_traceback_in_non_main_thread(capsys):
    """Signal handler registration should be silently skipped in non-main threads.

    Regression test for https://github.com/crewAIInc/crewAI/issues/4289
    """
    errors: list[Exception] = []

    def init_in_thread():
        try:
            t = Telemetry()
            t._register_shutdown_handlers()
        except Exception as exc:
            errors.append(exc)

    thread = threading.Thread(target=init_in_thread)
    thread.start()
    thread.join()

    assert not errors, f"Unexpected error: {errors}"
    captured = capsys.readouterr()
    assert "ValueError" not in captured.err
    assert "signal only works in main thread" not in captured.err


Review comment (bug):
Using `capsys` is not very deterministic. A better approach would be to mock `logger.debug` and assert it's never called, or mock `signal.signal` and assert it's never called when off the main thread.

Code after:
    for thread in threads:
        thread.join()

    assert all(instance is telemetry1 for instance in instances)


def test_no_signal_handler_traceback_in_non_main_thread():
    """Signal handler registration should be silently skipped in non-main threads.

    Regression test for https://github.com/crewAIInc/crewAI/issues/4289
    """
    errors: list[Exception] = []
    mock_holder: dict = {}

    def init_in_thread():
        try:
            Telemetry._instance = None
            with (
                patch.dict(
                    os.environ,
                    {"CREWAI_DISABLE_TELEMETRY": "false", "OTEL_SDK_DISABLED": "false"},
                ),
                patch("crewai.telemetry.telemetry.TracerProvider"),
                patch("signal.signal") as mock_signal,
                patch("crewai.telemetry.telemetry.logger") as mock_logger,
            ):
                Telemetry()
                mock_holder["signal"] = mock_signal
                mock_holder["logger"] = mock_logger
        except Exception as exc:
            errors.append(exc)

    thread = threading.Thread(target=init_in_thread)
    thread.start()
    thread.join()

    assert not errors, f"Unexpected error: {errors}"
    assert mock_holder, "Thread did not execute"
    mock_holder["signal"].assert_not_called()
    mock_holder["logger"].debug.assert_any_call(
        "Skipping signal handler registration: not running in main thread"
    )
Repository: danielgindi/Charts
File: Source/Charts/Data/Interfaces/IChartDataSet.swift (Swift)

Code before:
//
//  IChartDataSet.swift
//  Charts
//
//  Copyright 2015 Daniel Cohen Gindi & Philipp Jahoda
//  A port of MPAndroidChart for iOS
//  Licensed under Apache License 2.0
//
//  https://github.com/danielgindi/Charts
//

import Foundation

@objc
public protocol IChartDataSet
{
    // MARK: - Data functions and accessors
    
    /// Use this method to tell the data set that the underlying data has changed
    func notifyDataSetChanged()
    
    /// Calculates the minimum and maximum x and y values (_xMin, _xMax, _yMin, _yMax).
    func calcMinMax()
    
    /// Calculates the min and max y-values from the Entry closest to the given fromX to the Entry closest to the given toX value.
    /// This is only needed for the autoScaleMinMax feature.
    func calcMinMaxY(fromX: Double, toX: Double)
    
    /// - returns: The minimum y-value this DataSet holds
    var yMin: Double { get }
    
    /// - returns: The maximum y-value this DataSet holds
    var yMax: Double { get }
    
    /// - returns: The minimum x-value this DataSet holds
    var xMin: Double { get }
    
    /// - returns: The maximum x-value this DataSet holds

Review comment (suggestion):
It's interesting in my Xcode, it never warns or complains missing `CoreGraphics`, though `IChartDataSet` refers `CGPoint`

Code after:
//
//  IChartDataSet.swift
//  Charts
//
//  Copyright 2015 Daniel Cohen Gindi & Philipp Jahoda
//  A port of MPAndroidChart for iOS
//  Licensed under Apache License 2.0
//
//  https://github.com/danielgindi/Charts
//

import Foundation
import CoreGraphics

@objc
public protocol IChartDataSet
{
    // MARK: - Data functions and accessors
    
    /// Use this method to tell the data set that the underlying data has changed
    func notifyDataSetChanged()
    
    /// Calculates the minimum and maximum x and y values (_xMin, _xMax, _yMin, _yMax).
    func calcMinMax()
    
    /// Calculates the min and max y-values from the Entry closest to the given fromX to the Entry closest to the given toX value.
    /// This is only needed for the autoScaleMinMax feature.
    func calcMinMaxY(fromX: Double, toX: Double)
    
    /// - returns: The minimum y-value this DataSet holds
    var yMin: Double { get }
    
    /// - returns: The maximum y-value this DataSet holds
    var yMax: Double { get }
    
    /// - returns: The minimum x-value this DataSet holds
    var xMin: Double { get }
Repository: danielgindi/Charts
File: Source/Charts/Charts/ChartViewBase.swift (Swift)

Code before:
    {
        var entry: ChartDataEntry?
        var h = highlight
        
        if h == nil
        {
            _indicesToHighlight.removeAll(keepingCapacity: false)
        }
        else
        {
            // set the indices to highlight
            entry = _data?.entryForHighlight(h!)
            if entry == nil
            {
                h = nil
                _indicesToHighlight.removeAll(keepingCapacity: false)
            }
            else
            {
                _indicesToHighlight = [h!]
            }
        }
        
        if callDelegate && delegate != nil
        {
            if h == nil
            {
                delegate?.chartValueNothingSelected?(self)
            }
            else
            {
                // notify the listener
                delegate?.chartValueSelected?(self, entry: entry!, highlight: h!)
            }
        }
        
        // redraw the chart
        setNeedsDisplay()
    }
    
    /// - returns: The Highlight object (contains x-index and DataSet index) of the
    /// selected value at the given touch point inside the Line-, Scatter-, or
    /// CandleStick-Chart.
    open func getHighlightByTouchPoint(_ pt: CGPoint) -> Highlight?
    {
        if _data === nil
        {
            Swift.print("Can't select by touch. No data set.")
            return nil
        }
        

Review comment (suggestion):
To follow the spirit of your PR, you should flip the order of the if-else, i.e.
```
if let h = h { ... }
else { ... }
```

Code after:
        }
        else
        {
            // set the indices to highlight
            entry = _data?.entryForHighlight(h!)
            if entry == nil
            {
                h = nil
                _indicesToHighlight.removeAll(keepingCapacity: false)
            }
            else
            {
                _indicesToHighlight = [h!]
            }
        }
        
        if callDelegate, let delegate = delegate
        {
            if let h = h
            {
                // notify the listener
                delegate.chartValueSelected?(self, entry: entry!, highlight: h)
            }
            else
            {
                delegate.chartValueNothingSelected?(self)
            }
        }
        
        // redraw the chart
        setNeedsDisplay()
    }
    
    /// - returns: The Highlight object (contains x-index and DataSet index) of the
    /// selected value at the given touch point inside the Line-, Scatter-, or
    /// CandleStick-Chart.
    @objc open func getHighlightByTouchPoint(_ pt: CGPoint) -> Highlight?
    {
        if _data === nil
        {
            Swift.print("Can't select by touch. No data set.")
            return nil
        }
        
        return self.highlighter?.getHighlight(x: pt.x, y: pt.y)
    }

    /// The last value that was highlighted via touch.
    @objc open var lastHighlighted: Highlight?
  
    // MARK: - Markers
Repository: danielgindi/Charts
File: Source/Charts/Charts/CombinedChartView.swift (Swift)

Code before:
            dataSetIndex: h.dataSetIndex,
            stackIndex: -1,
            axis: h.axis)
    }
    
    // MARK: - CombinedChartDataProvider
    
    open var combinedData: CombinedChartData?
    {
        get
        {
            return _data as? CombinedChartData
        }
    }
    
    // MARK: - LineChartDataProvider
    
    open var lineData: LineChartData?
    {
        get
        {
            if _data === nil
            {
                return nil
            }
            return (_data as? CombinedChartData)?.lineData
        }
    }
    
    // MARK: - BarChartDataProvider
    
    open var barData: BarChartData?
    {
        get
        {
            if _data === nil
            {
                return nil
            }
            return (_data as? CombinedChartData)?.barData
        }
    }
    
    // MARK: - ScatterChartDataProvider
    
    open var scatterData: ScatterChartData?
    {
        get
        {
            if _data === nil
            {

Review comment (suggestion):
It has been sometime we review this part. But seems `CombinedChartData!` can be `CombinedChartData` now, 

Code after:
            dataSetIndex: h.dataSetIndex,
            stackIndex: -1,
            axis: h.axis)
    }
    
    // MARK: - CombinedChartDataProvider
    
    open var combinedData: CombinedChartData?
    {
        get
        {
            return _data as? CombinedChartData
        }
    }
    
    // MARK: - LineChartDataProvider
    
    open var lineData: LineChartData?
    {
        get
        {
            return combinedData?.lineData
        }
    }
    
    // MARK: - BarChartDataProvider
    
    open var barData: BarChartData?
    {
        get
        {
            return combinedData?.barData
        }
    }
    
    // MARK: - ScatterChartDataProvider
    
    open var scatterData: ScatterChartData?
    {
        get
        {
            return combinedData?.scatterData
        }
    }
    
    // MARK: - CandleChartDataProvider
    
    open var candleData: CandleChartData?
    {
        get
        {
Repository: danielgindi/Charts
File: Source/Charts/Charts/CombinedChartView.swift (Swift)

Code before:
            dataSetIndex: h.dataSetIndex,
            stackIndex: -1,
            axis: h.axis)
    }
    
    // MARK: - CombinedChartDataProvider
    
    open var combinedData: CombinedChartData?
    {
        get
        {
            return _data as? CombinedChartData
        }
    }
    
    // MARK: - LineChartDataProvider
    
    open var lineData: LineChartData?
    {
        get
        {
            if _data === nil
            {
                return nil
            }
            return (_data as? CombinedChartData)?.lineData
        }
    }
    
    // MARK: - BarChartDataProvider
    
    open var barData: BarChartData?
    {
        get
        {
            if _data === nil
            {
                return nil
            }
            return (_data as? CombinedChartData)?.barData
        }
    }
    
    // MARK: - ScatterChartDataProvider
    
    open var scatterData: ScatterChartData?
    {
        get
        {
            if _data === nil
            {

Review comment (suggestion):
shall we use `(_data as! CombinedChartData)` or `(_data as? CombinedChartData)?` ? seems `as!` is safe here though as? is safer

Code after:
            dataSetIndex: h.dataSetIndex,
            stackIndex: -1,
            axis: h.axis)
    }
    
    // MARK: - CombinedChartDataProvider
    
    open var combinedData: CombinedChartData?
    {
        get
        {
            return _data as? CombinedChartData
        }
    }
    
    // MARK: - LineChartDataProvider
    
    open var lineData: LineChartData?
    {
        get
        {
            return combinedData?.lineData
        }
    }
    
    // MARK: - BarChartDataProvider
    
    open var barData: BarChartData?
    {
        get
        {
            return combinedData?.barData
        }
    }
    
    // MARK: - ScatterChartDataProvider
    
    open var scatterData: ScatterChartData?
    {
        get
        {
            return combinedData?.scatterData
        }
    }
    
    // MARK: - CandleChartDataProvider
    
    open var candleData: CandleChartData?
    {
        get
        {
Repository: danielgindi/Charts
File: Source/Charts/Charts/CombinedChartView.swift (Swift)

Code before:
    {
        super.initialize()
        
        self.highlighter = CombinedHighlighter(chart: self, barDataProvider: self)
        
        // Old default behaviour
        self.highlightFullBarEnabled = true
        
        _fillFormatter = DefaultFillFormatter()
        
        renderer = CombinedChartRenderer(chart: self, animator: _animator, viewPortHandler: _viewPortHandler)
    }
    
    open override var data: ChartData?
    {
        get
        {
            return super.data
        }
        set
        {
            super.data = newValue
            
            self.highlighter = CombinedHighlighter(chart: self, barDataProvider: self)
            
            (renderer as? CombinedChartRenderer)?.createRenderers()
            renderer?.initBuffers()
        }
    }
    
    open var fillFormatter: IFillFormatter
    {
        get
        {
            return _fillFormatter
        }
        set
        {
            _fillFormatter = newValue
            if _fillFormatter == nil
            {
                _fillFormatter = DefaultFillFormatter()
            }
        }
    }
    
    /// - returns: The Highlight object (contains x-index and DataSet index) of the selected value at the given touch point inside the CombinedChart.
    open override func getHighlightByTouchPoint(_ pt: CGPoint) -> Highlight?
    {
        if _data === nil
        {

Review comment (bug):
old code is a bit weird (maybe the compiler bug). But if it's safe unwrap here, why not use `(renderer as! CombinedChartRenderer).createRenderers()`? `renderer` has been setup as combined chart renderer in `initialize()` above.

Code after:
    {
        super.initialize()
        
        self.highlighter = CombinedHighlighter(chart: self, barDataProvider: self)
        
        // Old default behaviour
        self.highlightFullBarEnabled = true
        
        _fillFormatter = DefaultFillFormatter()
        
        renderer = CombinedChartRenderer(chart: self, animator: _animator, viewPortHandler: _viewPortHandler)
    }
    
    open override var data: ChartData?
    {
        get
        {
            return super.data
        }
        set
        {
            super.data = newValue
            
            self.highlighter = CombinedHighlighter(chart: self, barDataProvider: self)
            
            (renderer as? CombinedChartRenderer)?.createRenderers()
            renderer?.initBuffers()
        }
    }
    
    @objc open var fillFormatter: IFillFormatter
    {
        get
        {
            return _fillFormatter
        }
        set
        {
            _fillFormatter = newValue
            if _fillFormatter == nil
            {
                _fillFormatter = DefaultFillFormatter()
            }
        }
    }
    
    /// - returns: The Highlight object (contains x-index and DataSet index) of the selected value at the given touch point inside the CombinedChart.
    open override func getHighlightByTouchPoint(_ pt: CGPoint) -> Highlight?
    {
        if _data === nil
        {
Repository: danielgindi/Charts
File: Source/Charts/Components/MarkerImage.swift (Swift)

Code before:
        // Do nothing here...
    }
    
    open func draw(context: CGContext, point: CGPoint)
    {
        let offset = self.offsetForDrawing(atPoint: point)
        
        var size = self.size
        
        if size.width == 0.0 && image != nil
        {
            size.width = image?.size.width ?? 0.0
        }
        if size.height == 0.0 && image != nil
        {
            size.height = image?.size.height ?? 0.0
        }
        
        let rect = CGRect(
            x: point.x + offset.x,
            y: point.y + offset.y,
            width: size.width,
            height: size.height)
        
        NSUIGraphicsPushContext(context)
        image?.draw(in: rect)
        NSUIGraphicsPopContext()
    }
}


Review comment (suggestion):
Seems like there should be a `guard let image = image else { return }` at the beginning of the method. Drawing will do nothing if `image == nil`

Code after:
        // Do nothing here...
    }
    
    open func draw(context: CGContext, point: CGPoint)
    {
        guard let image = image else { return }

        let offset = offsetForDrawing(atPoint: point)
        
        var size = self.size
        
        if size.width == 0.0
        {
            size.width = image.size.width
        }
        if size.height == 0.0
        {
            size.height = image.size.height
        }
        
        let rect = CGRect(
            x: point.x + offset.x,
            y: point.y + offset.y,
            width: size.width,
            height: size.height)
        
        NSUIGraphicsPushContext(context)
        image.draw(in: rect)
        NSUIGraphicsPopContext()
    }
}
Repository: danielgindi/Charts
File: Source/Charts/Data/Implementations/Standard/BubbleChartData.swift (Swift)

Code before:
//  Bubble chart implementation:
//    Copyright 2015 Pierre-Marc Airoldi
//    Licensed under Apache License 2.0
//
//  https://github.com/danielgindi/Charts
//

import Foundation
import CoreGraphics

open class BubbleChartData: BarLineScatterCandleBubbleChartData
{
    public override init()
    {
        super.init()
    }
    
    public override init(dataSets: [IChartDataSet]?)
    {
        super.init(dataSets: dataSets)
    }
    
    /// Sets the width of the circle that surrounds the bubble when highlighted for all DataSet objects this data object contains
    @objc open func setHighlightCircleWidth(_ width: CGFloat)
    {
        (_dataSets as? [IBubbleChartDataSet])?.forEach { set in
            set.highlightCircleWidth = width
        }
    }
}


Review comment (suggestion):
Could be a one liner as 
`(_dataSets as? [IBubbleChartDataSet])?.forEach { $0.highlightCircleWidth = width }`

Code after:
//  Bubble chart implementation:
//    Copyright 2015 Pierre-Marc Airoldi
//    Licensed under Apache License 2.0
//
//  https://github.com/danielgindi/Charts
//

import Foundation
import CoreGraphics

open class BubbleChartData: BarLineScatterCandleBubbleChartData
{
    public override init()
    {
        super.init()
    }
    
    public override init(dataSets: [IChartDataSet]?)
    {
        super.init(dataSets: dataSets)
    }
    
    /// Sets the width of the circle that surrounds the bubble when highlighted for all DataSet objects this data object contains
    @objc open func setHighlightCircleWidth(_ width: CGFloat)
    {
        (_dataSets as? [IBubbleChartDataSet])?.forEach { $0.highlightCircleWidth = width }
    }
}
Repository: danielgindi/Charts
File: Source/Charts/Data/Implementations/Standard/ChartData.swift (Swift)

Code before:
    {
        let element = self._dataSets.removeFirst()
        notifyDataChanged()
        return element
    }

    public func removeFirst(_ n: Int)
    {
        self._dataSets.removeFirst(n)
        notifyDataChanged()
    }

    public func removeLast() -> Element
    {
        let element = self._dataSets.removeLast()
        notifyDataChanged()
        return element
    }

    public func removeLast(_ n: Int)
    {
        self._dataSets.removeLast(n)
        notifyDataChanged()
    }

//    public func removeSubrange(_ bounds: Range<Index>) {
//        self.dataSets.removeSubrange(bounds)
//        notifyDataChanged()
//    }
}

// MARK: Swift Accessors
extension ChartData
{
    //TODO: Reevaluate if warning is still true
    /// Retrieve the index of a ChartDataSet with a specific label from the ChartData. Search can be case sensitive or not.
    /// **IMPORTANT: This method does calculations at runtime, do not over-use in performance critical situations.**
    ///
    /// - Parameters:
    ///   - label: The label to search for
    ///   - ignoreCase: if true, the search is not case-sensitive
    /// - Returns: The index of the DataSet Object with the given label. `nil` if not found
    public func index(forLabel label: String, ignoreCase: Bool) -> Index?
    {
        return ignoreCase
            ? index { $0.label?.caseInsensitiveCompare(label) == .orderedSame }
            : index { $0.label == label }
    }

    public subscript(label: String, ignoreCase: Bool) -> Element?
    {

Review comment (suggestion):
don't need to call notifyDatChanged here()? how about in `replaceSubrange`. Shall we consider add some unit tests for these type of changes.

Code after:
        }

        let element = _dataSets.remove(at: position)
        calcMinMax()
        return element
    }

    public func removeFirst() -> Element
    {
        guard !(self is CombinedChartData) else
        {
            fatalError("removeFirst() not supported for CombinedData")
        }

        let element = _dataSets.removeFirst()
        notifyDataChanged()
        return element
    }

    public func removeFirst(_ n: Int)
    {
        guard !(self is CombinedChartData) else
        {
            fatalError("removeFirst(_:) not supported for CombinedData")
        }

        _dataSets.removeFirst(n)
        notifyDataChanged()
    }

    public func removeLast() -> Element
    {
        guard !(self is CombinedChartData) else
        {
            fatalError("removeLast() not supported for CombinedData")
        }

        let element = _dataSets.removeLast()
        notifyDataChanged()
        return element
    }

    public func removeLast(_ n: Int)
    {
        guard !(self is CombinedChartData) else
        {
            fatalError("removeLast(_:) not supported for CombinedData")
        }

        _dataSets.removeLast(n)
        notifyDataChanged()