Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort position vs element equality #34

Open
adamwulf opened this issue Jan 9, 2023 · 1 comment
Open

Sort position vs element equality #34

adamwulf opened this issue Jan 9, 2023 · 1 comment

Comments

@adamwulf
Copy link

adamwulf commented Jan 9, 2023

I was surprised to find that the sort position of an element is also being used to determine element equality. This is can be seen with the following test case:

    func testEqualityVsSortPosition() {
        struct Item: Equatable {
            let id: String
            let name: String
        }

        let item1 = Item(id: "1", name: "Bumble")
        let item2 = Item(id: "2", name: "Bumble") // two items with same name, but not Equality
        let item3 = Item(id: "3", name: "Fumble")
        let item4 = Item(id: "4", name: "Grumble")

        // array is missing item 2
        var array = SortedArray(unsorted: [item1, item3, item4], areInIncreasingOrder: { $0.name < $1.name })

        // correctly find item 1
        XCTAssertTrue(array.contains(item1))       // passes
        // incorrectly find item 2
        XCTAssertFalse(array.contains(item2))      // fails

        XCTAssertNil(array.index(of: item2))       // fails
        XCTAssertNil(array.anyIndex(of: item2))    // fails
        XCTAssertNil(array.firstIndex(of: item2))  // fails
        XCTAssertNil(array.lastIndex(of: item2))   // fails

        let countBefore = array.count
        array.remove(item2)
        let countAfter = array.count
        XCTAssertEqual(countBefore, countAfter) // fails
    }

I would expect methods like remove(_ element: Element) to only remove if the elements are actually equal, not just if they're sorted into the same place. This method has the same signature as the one on Set, but behaves very differently.

A workaround is to define the sort function to take equality into account. I don't believe it's intuitive to consider objects sorted to the same index as also being Equal. At a minimum this should be better documented. I'd suggest to either update the functionality to take Equatable conformance into account, or change the method names to make it clear its acting on sort order not equality.

@ole
Copy link
Owner

ole commented Jan 11, 2023

@adamwulf Thanks for reporting this. I'll take a look, but it may take some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants