Flatten (Issue #356) #357

leandromoh · 2017-10-12T18:58:34Z

Implementation for #356.

atifaziz

Please fix broken build/test. Thanks!

atifaziz · 2017-10-15T08:35:16Z

MoreLinq.Test/FlattenTest.cs

+
+            var result = source.Flatten();
+
+            Assert.AreEqual(13, result.Count());


Change assertions to use the more readable constraints approach:

Assert.That(result.Count(), Is.EqualTo(13));

atifaziz · 2017-10-15T08:35:20Z

MoreLinq.Test/FlattenTest.cs

+        [Test]
+        public void FlattenOfType()
+        {
+            var source = new List<object>()


Use an array instead of List<> here and in other similar cases. A list isn't needed, distracting & verbose.

atifaziz · 2017-10-15T08:36:01Z

MoreLinq.Test/FlattenTest.cs

+    public class FlattenTest
+    {
+        [Test]
+        public void FlattenOfType()


This name doesn't mean anything.

atifaziz · 2017-10-15T08:45:23Z

MoreLinq/Flatten.cs

+        /// </returns>
+        /// <exception cref="ArgumentNullException"><paramref name="source"/> is null.</exception>
+        /// <exception cref="ArgumentNullException"><paramref name="isAtom"/> is null.</exception>
+        public static IEnumerable<object> Flatten(this IEnumerable source, Func<object, bool> isAtom)


isAtom should be renamed to predicate and the condition inverted such that returning true will flatten the inner sequence in question. I would also change the signature to Func<IEnumerable, bool>.

We can also look at this as a selector/predicate combined? The function signature would become:

public static IEnumerable<object> Flatten(this IEnumerable source, Func<object, IEnumerable> selector)

If the selector function returns null then it is skipped. The benefit of this version is that it would allow flattening via a property of an object when the object itself isn't iterable. The downside is that it overloads the selector function to also serve as a predicate and which won't be obvious from the signature (since we don't have an optional type in .NET).

atifaziz · 2017-10-15T08:54:11Z

MoreLinq/Flatten.cs

+                {
+                    if (e is IEnumerable inner && !isAtom(inner))
+                    {
+                        foreach (var i in _(inner))


How about coding an explicit stack here instead of relying on recursion?

atifaziz · 2017-10-15T09:17:35Z

MoreLinq.Test/NullArgumentTest.cs

+
+            return typeInfo.IsGenericType
+                    ? CreateGenericInterfaceInstance(typeInfo)
+                    : CreateNonGenericInterfaceInstance(typeInfo);


I don't understand the purpose of CreateNonGenericInterfaceInstance. Why not just use new NonGenericArgs.Enumerable()?

I tried to follow the pattern, but really it is not necessary here

atifaziz · 2017-10-15T09:19:12Z

MoreLinq.Test/FlattenTest.cs

+
+            Assert.AreEqual(13, result.Count());
+
+            result.OfType<int>().AssertSequenceEqual(Enumerable.Range(1, 10));


These assertions are not very clear and seem like a shortcut. They don't assert the order in which elements are returned in result. It's much clearer to write the full expected result.

atifaziz

I made some improvements directly to speed up things. There are just some tests for disposals missing now and then I believe we'll be done.

Thanks!

atifaziz · 2017-10-23T15:30:42Z

MoreLinq/Flatten.cs

+
+                        if (!next)
+                        {
+                            (e as IDisposable)?.Dispose();


Test to exercise this disposal is missing.

atifaziz · 2017-10-23T15:31:20Z

MoreLinq/Flatten.cs

+                {
+                    stack.Prepend(e)
+                         .OfType<IDisposable>()
+                         .ForEach(x => x.Dispose());


Test to exercise these disposals in the event of an exception is missing.

leandromoh · 2017-10-25T02:00:25Z

I made some improvements directly to speed up things. There are just some tests for disposals missing now and then I believe we'll be done.

Thanks for the help! Surely it speed up.

Tests for dispose were delivery.

leandromoh · 2017-11-03T11:20:34Z

@atifaziz I finished the development. I added the overload for map an object to a property. IMHO It is only missing the parameter documentation for "selector"

Singular of "series" is "series", not "serie".

atifaziz

I added the overload for map an object to a property.

I don't think that was the idea. What I said in my earlier suggestion was:

We can also look at this as a selector/predicate combined? The function signature would become:

public static IEnumerable<object> Flatten(this IEnumerable source, Func<object, IEnumerable> selector)

If the selector function returns null then it is skipped. The benefit of this version is that it would allow flattening via a property of an object when the object itself isn't iterable. The downside is that it overloads the selector function to also serve as a predicate and which is not obvious from the signature (since we don't have an optional type in .NET).

Your signature for the selector function is Func<object, object> and then for the predicate function it's Func<IEnumerable, bool>. Looking at just those two, it's not obvious what's going on, especially with the selector (projecting an object to yet another object?). At the very least, the selector should be Func<object, IEnumerable> because it's not surprising to see a projection going from an object (item of a parent sequence) to a (sub-)sequence in the context of a flattening operation. Since an IEnumerable is a reference and we'd have to deal with them being returned as null, I followed that we could use that as a predicate too, for example, to skip some iterables like strings. However, that was me just making a suggestion and was hoping @fsateler and/or you would chime in and we'd reach a consensus on the final design before updating the PR. As it stands right now, I feel the overload worsens things.

atifaziz · 2017-11-07T08:21:09Z

MoreLinq.Test/FlattenTest.cs

+
+            using (var test = source.AsTestingSequence())
+            {
+                Assert.That(test.Flatten(), Is.EquivalentTo(expectations));


Why not use AssertSequenceEqual to be consistent as in other tests? That is:

test.Flatten().AssertSequenceEqual(expectations);

atifaziz · 2017-11-07T08:27:24Z

MoreLinq.Test/FlattenTest.cs

+                Assert.That(test.Flatten(), Is.EquivalentTo(expectations));
+            }
+
+            new object[] { inner1, inner2, inner3 }.Cast<IDisposable>().ForEach(seq => seq.Dispose());


Two things. First, why not type the array elements as IDisposable in the first place instead of casting later?

new IDisposable[] { inner1, inner2, inner3 }.ForEach(seq => seq.Dispose())

Second, why not just use using like below?

[Test] public void FlattenFullIteratedDisposesInnerSequences() { using (var inner1 = TestingSequence.Of(4, 5)) using (var inner2 = TestingSequence.Of(true, false)) using (var inner3 = TestingSequence.Of<object>(6, inner2, 7)) { var source = new object[] { inner1, inner3, }; var expectations = new object[] { 4, 5, 6, true, false, 7, }; using (var test = source.AsTestingSequence()) test.Flatten().AssertSequenceEqual(expectations); } }

leandromoh · 2017-12-26T02:52:32Z

@atifaziz sorry, I thought had undertood your idea but I just did it now. I dont like too much about the idea of return null to indicate predicate (as you said, it is not an obvious signature).

How about maintain predicate and selector separated but put selector as Func<object, IEnumerable>? If we can not reach consensus on the final design soon, I suggest merge the PR with the MVP (predicate only) and discuss about selector in a separate thread (new issue).

leandromoh · 2017-12-28T11:27:11Z

@atifaziz what about the above question?

atifaziz · 2017-12-28T12:15:20Z

I suggest merge the PR with the MVP (predicate only) and discuss about selector in a separate thread (new issue).

Let's shoot for this. Doing an MVP is a good thing anyway.

leandromoh · 2017-12-30T01:21:33Z

@atifaziz The PR is only with the MVP now. ready for merge. can we publish a new release before december finished?

atifaziz · 2018-01-03T15:08:07Z

can we publish a new release before december finished?

Sorry but was AFK, celebrating.

I'll try and look at it this week.

leandromoh · 2018-01-10T14:20:53Z

Any news?

atifaziz · 2018-01-11T07:27:24Z

Did you rebase and remove commits because that's confused me a bit and we may have lost some work. The history of this PR seems to have diverged at d0297a5. I think it would have been simpler to git revert if you wanted to back out of changes. It also doesn't help if commit messages read “improvements” as it makes the review slower because then I need to go through the physical diff changes and figure out what they're about. All this means that a review that should have taken 5 minutes took 15 and then I had to abort and get back to other work until I have more time to spare.

leandromoh · 2018-01-11T10:44:05Z

Did you rebase and remove commits

I checkout to the last commit without selector overload. But before that, created a new branch with all work (this one for the possíble new PR)

It also doesn't help if commit messages read “improvements” as it makes the review slower

Sorry about the commit messages, i will be more descritive next time.

atifaziz · 2018-01-11T16:36:22Z

I checkout to the last commit without selector overload. But before that, created a new branch with all work (this one for the possíble new PR)

Okay, that explains it and so the branch was rewound. As I said, a revert would have been the right call here.

Sorry about the commit messages, i will be more descritive next time.

Cheers! 👍

This revert is a combination of the following: - Revert "Use arrays instead of lists" This reverts commit c8f59eb. - Revert "Fix grammar" This reverts commit 78fd850. - Revert "Delete trailing whitespace" This reverts commit c38da12. - Revert "fix" This reverts commit 0db744d. - Revert "improved" This reverts commit 68b7a9f. - Revert "fix" This reverts commit 977193a. - Revert "add selector overload" This reverts commit 9480d35.

atifaziz · 2018-01-11T20:39:17Z

@leandromoh I've put your changes on top of a revert (c6f59f6) just to maintain the “selector overload” as part of this PR's history and discussion, in case we need to pick it up again.

leandromoh · 2018-01-11T21:44:18Z

Ok. Thanks.

What about the code itself? Are we ready to merge?

atifaziz

The finally block needs a final touch/revision so almost there! 🏁

atifaziz · 2018-01-15T09:11:11Z

MoreLinq/Flatten.cs

+                }
+                finally
+                {
+                    stack.Prepend(e)


The finally block should be simple rather than use any fancy LINQ to avoid causing allocations under low memory conditions (the reason for which the finally code could be running in the first place). You can re-write this in the same lines of code:

(e as IDisposable)?.Dispose(); foreach (var se in stack) (se as IDisposable)?.Dispose();

Note that since Stack<>'s enumerator is a struct, there's no cost to iterating over it using foreach.

leandromoh · 2018-01-20T17:13:22Z

@atifaziz code refactored! also I added flatten to readme and release notes.

added flatten

87351cf

atifaziz requested changes Oct 12, 2017

View reviewed changes

leandromoh added 3 commits October 14, 2017 13:49

fix tests and created overload

dd561ab

commented assert with nested sequence

c57ecbf

fix NullArgumentTest

b7d9cc9

atifaziz requested changes Oct 15, 2017

View reviewed changes

leandromoh and others added 12 commits October 23, 2017 01:06

improviments

7058eea

fix tests, after predicate change

bee1272

Move variable closer to usage scope

d015cd6

Remove redundant ()s

27311fb

Remove unused import

e3651b1

Delete trailing spaces in test data

3d2c065

Replace lists with arrays

b86d5d5

Documentation review

7513f7d

Use expression body for the simple & delegating overload

3d71d24

Make re-looping explicit

7e6d023

Fix indentation

5baa88a

Clean-up NonGenericArgs.Enumerable as an empty enumerable

e51fb4e

atifaziz requested changes Oct 23, 2017

View reviewed changes

leandromoh added 7 commits October 25, 2017 01:20

created dispose tests

14b5c63

fix: cast sequence to IDisposable

3dc7d61

added Assert.Throws

d0297a5

add selector overload

9480d35

fix

977193a

improved

68b7a9f

fix

0db744d

atifaziz added 2 commits November 7, 2017 09:18

Delete trailing whitespace

c38da12

Fix grammar

78fd850

Singular of "series" is "series", not "serie".

atifaziz requested changes Nov 7, 2017

View reviewed changes

leandromoh force-pushed the flatten branch from c8f59eb to d0297a5 Compare December 30, 2017 00:06

leandromoh added 2 commits December 30, 2017 01:11

improviments

0427f4c

final improviments

9ef1281

atifaziz and others added 3 commits January 11, 2018 21:30

improviments

d002b19

final improviments

645368e

atifaziz force-pushed the flatten branch from 9ef1281 to 645368e Compare January 11, 2018 20:36

atifaziz added 2 commits January 15, 2018 09:59

Delete trailing whitespace

bdc5bf8

Use MoreEnumerable.From to simulate partway failing sequence

7916863

atifaziz requested changes Jan 15, 2018

View reviewed changes

leandromoh added 3 commits January 20, 2018 16:59

merge

8a7c54c

simplyfy finally with foreach

a60718c

added Flatten on README.md and release notes

f7c02fd

atifaziz approved these changes Jan 21, 2018

View reviewed changes

atifaziz merged commit b82b5b7 into morelinq:master Jan 21, 2018

leandromoh mentioned this pull request Mar 12, 2018

Flatten (overload) via property of an object #413

Closed

leandromoh deleted the flatten branch April 1, 2018 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatten (Issue #356) #357

Flatten (Issue #356) #357

leandromoh commented Oct 12, 2017 •

edited by atifaziz

atifaziz left a comment

atifaziz Oct 15, 2017

atifaziz Oct 15, 2017

atifaziz Oct 15, 2017

atifaziz Oct 15, 2017 •

edited

atifaziz Oct 15, 2017

atifaziz Oct 15, 2017

leandromoh Oct 23, 2017

atifaziz Oct 15, 2017

atifaziz left a comment

atifaziz Oct 23, 2017

atifaziz Oct 23, 2017

leandromoh commented Oct 25, 2017

leandromoh commented Nov 3, 2017

atifaziz left a comment •

edited

atifaziz Nov 7, 2017

atifaziz Nov 7, 2017

leandromoh commented Dec 26, 2017 •

edited

leandromoh commented Dec 28, 2017

atifaziz commented Dec 28, 2017

leandromoh commented Dec 30, 2017

atifaziz commented Jan 3, 2018

leandromoh commented Jan 10, 2018

atifaziz commented Jan 11, 2018

leandromoh commented Jan 11, 2018 •

edited

atifaziz commented Jan 11, 2018

atifaziz commented Jan 11, 2018

leandromoh commented Jan 11, 2018

atifaziz left a comment

atifaziz Jan 15, 2018

leandromoh commented Jan 20, 2018


		var result = source.Flatten();

		Assert.AreEqual(13, result.Count());


		Assert.AreEqual(13, result.Count());

		result.OfType<int>().AssertSequenceEqual(Enumerable.Range(1, 10));

Flatten (Issue #356) #357

Flatten (Issue #356) #357

Conversation

leandromoh commented Oct 12, 2017 • edited by atifaziz

atifaziz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atifaziz Oct 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atifaziz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandromoh commented Oct 25, 2017

leandromoh commented Nov 3, 2017

atifaziz left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandromoh commented Dec 26, 2017 • edited

leandromoh commented Dec 28, 2017

atifaziz commented Dec 28, 2017

leandromoh commented Dec 30, 2017

atifaziz commented Jan 3, 2018

leandromoh commented Jan 10, 2018

atifaziz commented Jan 11, 2018

leandromoh commented Jan 11, 2018 • edited

atifaziz commented Jan 11, 2018

atifaziz commented Jan 11, 2018

leandromoh commented Jan 11, 2018

atifaziz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandromoh commented Jan 20, 2018

leandromoh commented Oct 12, 2017 •

edited by atifaziz

atifaziz Oct 15, 2017 •

edited

atifaziz left a comment •

edited

leandromoh commented Dec 26, 2017 •

edited

leandromoh commented Jan 11, 2018 •

edited