add index_of, last_index_of, and for_each

[mzhukovs]

PR for #70 for your consideration

[viralogic]

@mzhukovs

I appreciate the PR. I apologise, but haven't had a chance to look at it yet. I will review it and get back to you with some comments as soon as I can.

[viralogic]

The method signature for this should include some lambda function to act as a selector for more complicated objects that do not have an __eq__ method defined (ie. how to define equality between 2 different instances of an object with same properties).

Also, what if your collection contains different types of objects (which is possible in an Enumerable collection). How do you define equality between these 2 different objects? This is what the lambda function would be for. As a default it could be set as lambda x: x

For example:

class Point(object):
    def __init__(self, x, y):
        self.x = x
        self.y = y

points = Enumerable([Point(1, 2), Point(1, 3)])
assert points.index_of(Point(1,2)) is not None

This above example would fail in the assert because Point(1,2) are two different instances of Point even though they have the same x, y values.

[mzhukovs]

Good point, I guess originally I was just leaving it up to them to implement eq or do a select first then index_of to account for what's needed beforehand, but an optional let's call it normalization function sounds like a good idea. I'll update in a bit.

[mzhukovs]

Bit of an impasse on this, issue #72 (not specifically with regards to any() though) makes the behavior of this difficult to predict.

Here's what I wanted to do:

Updated index_of function:

    def index_of(self, element, func=lambda x: x, apply_func_to_element=False):
        """
        Returns the index of the first occurrence of a given element.

        :param element: the element for which to retrieve the index
        :param func: optional selector to apply to the collection as lambda expression for equality comparison
        :param apply_func_to_element: optional boolean flag indicating whether the selector lambda should be applied to the passed in element as well - default is False
        :return: Index of given element
        """
        for i, e in enumerate(self):
            if func(e) == (func(element) if apply_func_to_element else element):
                return i

        return None

Set up for testing:

class Rectangle:
    def __init__(self, length, width):
        self.length = length
        self.width = width

    def __eq__(self, other):
        if isinstance(other, Rectangle):
            return self.length == other.length and self.width == other.width

    def area(self):
        return self.length * self.width

    def perimeter(self):
        return 2 * self.length + 2 * self.width


class Square(Rectangle):
    def __init__(self, length):
        super().__init__(length, length)

self.complex_types = Enumerable([
    Rectangle(1, 2),
    Rectangle(1, 4),
    Rectangle(2, 1),
    Rectangle(3, 4),
    Square(1),
    Square(2),
])

Test:

    def test_index_of_complex_types(self):
        self.assertEqual(4, self.complex_types.index_of(Square(1)))
        self.assertEqual(1, self.complex_types.index_of(4, lambda x: x.area()))
        self.assertEqual(1, self.complex_types.index_of(Square(2), lambda x: x.area(), True)     

Those assertions would pass if they were the only ones being executed, but since the Enumerable's order gets changed after a call to any of them, index_of becomes unreliable thereafter.

    def test_index_of_complex_types(self):
        # issue #72 will leave to undesirable behavior without an order_by happening first
        complex_types = self.complex_types.order_by(lambda x: (x.length, x.width))
        # -> [ Square(1), Rectangle(1,2), Rectangle(1,4), Rectangle(2,1), Square(2), Rectangle(3,4)]
        self.assertEqual(2, complex_types.index_of(4, lambda x: x.area()))

        # have to do it again unfortunately
        complex_types = self.complex_types.order_by(lambda x: (x.length, x.width))
        self.assertEqual(2, complex_types.index_of(Square(2), lambda x: x.area(), True))

What are your thoughts on this?

[viralogic]

Same comments as for index_of

[viralogic]

Looking at this code, it would be an executing function when it probably shouldn't be as there is no need. This would have to be tested, but I think this function could just be a wrapper around the output from the select function. For example:

def for_each(self, func=lambda x: x):
    return self.select(func)

[mzhukovs]

That was intentional, basically replicating this where input is Action and return is void: https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.list-1.foreach?view=net-6.0

[viralogic]

Left some comments on your code. Code reviews are always hard. I really appreciate your work on this. I think if the requested changes are made and all the tests pass, I can accept your PR!

[mzhukovs]

Think there needs to be a bit of a refactoring to make order consistent, otherwise implementing index_of, or even the existing implementation of first() becomes a moot point.

e.g. first() isn't deterministic right now, call it 2x in a row on the same Enumerable and you'll get 2 different outputs.

[viralogic]

I agree with you here

…

On Wed., Jan. 12, 2022, 10:43 p.m. Mark Zhukovsky, ***@***.***> wrote: Think there needs to be a bit of a refactoring to make order consistent, otherwise implementing index_of, or even the existing implementation of first() becomes a moot point. e.g. first() isn't deterministic right now, call it 2x in a row on the same Enumerable and you'll get 2 different outputs. — Reply to this email directly, view it on GitHub <#71 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOO5MFS6O2JCRFOKCZ6JA3UVZRBDANCNFSM5GDJXGKA> . You are receiving this because you commented.Message ID: ***@***.***>

[viralogic]

@mzhukovs

Again, apologies. New job is keeping me busy, but I haven't forgotten about this. Just as an FYI, I have created issue #73 to deal with the issue you have raised and plan on fixing it soon.

Are you ok with both of us having a look at this PR once I get the fix for Issue #73 into development?

[mzhukovs]

@viralogic definitely - would be great to circle back!