Closed Smoren closed 1 year ago
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Totals | |
---|---|
Change from base Build 2224581354: | 0.2% |
Covered Lines: | 163 |
Relevant Lines: | 164 |
Hi @Smoren,
Thank you for your interest in IterTools and your kind words.
Up until now, all of the methods are providing functionality to iterate, so the call to a function returns something that itself can be iterated.
Reduce is interesting, because the result is a single value, so the call to the function doesn't return an iterator, but the result of iteration and reduction.
I think it makes sense to create a new namespace class for this, since it is different than current iteration categories. How about a class IterTools\Reduce
and the method that you are submitting can be Reduce::toValue(...)
.
There are other reductions I am considering, such as Reduce::toLength
, Reduce::toMin/Max
, Reduce::isSorted
, Reduce::exactlyN
etc., so I think having it's own namespace class is the right place to put this functionality since it is different from other namespaces that provide iteration rather than single values.
Some tips for the unit tests: Structure the tests in the Given/When/Then format using those comments. Take a look at how other tests are formatted. Also, when asserting an expected value, have the result be precomputed. Do not use code in a unit test. We write unit tests to validate our code. We don't want to use code to validate code--we want to use specific answers to validate code. Again, take a look at other tests in this library to see how results are precomputed by hand and fed into assertions as expected answers. Also, try to think of edge/boundary cases: empty list, list of single value, carry not provided, (should carry be required?).
Thanks again for your pull request and introducing some new functionality. Mark
Thank you for your feedback!
Hope it's done right now.
Single::reduce()
removedReduce
created with methods:
ArrayIteratorFixture
at line 50. See comment there.I think you will need to add a new Reduce test directory to the PHPUnit XML configuration for tests and code coverage to run on the new code. See: https://github.com/markrogoyski/itertools-php/blob/main/tests/phpunit.xml
Done. I'm sorry, I'm used to the codeception, so I didn't figure it out right away.
Hi!
I've optimized method Reduce::toCount() for arrays and Countable instances. Now this method uses count() function for them, so complexity reduced from O(N) to O(1) if we get array or Countable as input.
Also I've added method Single::eachPair() for iterating pair of elements in collection. I need this method to implement Reduce::isSorted().
A few more methods added:
I hope you think I'm still on the right way :)
@markrogoyski
Hi!
I'm sorry, I've made a lot of commits, so I decided to make a short report so as not to waste a lot of your time.
There will be no more commits to this branch until your comments are received. Ideas for new methods will be worked out separately.
Reduce::toValue()
;Reduce::toMin()
;Reduce::toMax()
;Reduce::toCount()
;Reduce::toSum()
;Reduce::toProduct()
;Reduce::toAverage()
;Reduce::isSortedDirectly()
;Reduce::isSortedReversely()
;Single::filterUnique()
;Single::eachPair()
.All methods have been documented (in PHPdoc and README.md) and tested similarly to yours.
During testing, it was necessary to introduce a new fixture — CountableIteratorAggregateFixture
, because the Reduce::toCount()
method works in a special way with Countable
entities (in order to optimize).
A bug was found and fixed in ArrayIteratorFixture::valid()
.
You used the isset()
function there, which led to the premature termination of the iteration process, when the iterable array contained null
(if $a[0] === null
then isset($a[0]) === false
).
I replaced isset()
with array_key_exists()
to fix this bug.
There is a comment in that method (file ArrayIteratorFixture.php
, line 50) that needs to be removed before accepting this pull request.
Single::eachPair()
to Single::pairs()
?Single::toCount()
to Single::toLength()
?I will be very happy about feedback. I hope that my pull request will be accepted and the new methods will be useful to the community.
I am ready for any comments and I will make any necessary changes to the code according to your wishes.
Thank you for your time!
Hi @Smoren,
Thank you for implementing additional methods to fill out the new Reduce
iter tools, and thanks for the summary of changes. I will review the code and leave any feedback I have.
One high-level comment. First off, thank you for adding unit tests. For tests that reduce to a boolean, such as the isSorted
methods, I think the tests will be easier to follow and allow for simpler data provider methods if the "true" and "false" test cases are separated into separate test methods with their own data providers. Doing this, the data provider is only the values, and the test can simply assertTrue
or assertFalse
. For example, separate IsSortedDirectlyTest
testArray
into two test case methods: testArrayTrue
and testArrayFalse
. No expected parameter would be required as the expectation is always the same and tested directly with the assertTrue
/assertFalse
methods.
Thanks again for adding additional functionality and following the coding standards. Mark
@markrogoyski
Hi! Thank you for your feedback!
I've made all the suggested changes. Here is the summary:
Single::eachPair()
renamed to Single::pairwise()
;Reduce::isSortedDirectly()
renamed to Reduce::isSorted()
;Reduce::isSortedReversely()
renamed to Reduce::isReversed()
;Reduce::toCount()
: used function is_countable()
;assertTrue()
and assertFalse()
for Reduce::isSorted()
and Reduce::isReverted()
;Single::filterUnique()
removed;assertEqualsWithDelta()
: ToAverageTest
, ToSumTest
, ToProductTest
.Hi @Smoren,
Thanks again for the pull request. I've merged it and will include it in the next release of IterTools.
Thanks again! Mark
@markrogoyski thank you very much! I will continue thinking about new methods :)
New method is fully covered by tests. It seemed to me that this method would be suitable for your beautiful repository. I hope you will agree with me. Thanks for this great package!