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

Use ^7.1 php with iterables #52

Closed
wants to merge 1 commit into from
Closed

Use ^7.1 php with iterables #52

wants to merge 1 commit into from

Conversation

enleur
Copy link
Contributor

@enleur enleur commented Dec 25, 2017

No description provided.

src/iter.php Outdated
@@ -295,15 +286,14 @@ function reductions(callable $function, $iterable, $startValue = null) {
* iter\zip([1, 2, 3], [4, 5, 6], [7, 8, 9])
* => iter([1, 4, 7], [2, 5, 8], [3, 6, 9])
*
* @param array|Traversable ...$iterables Iterables to zip
* @param iterable[] ...$iterables
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be just iterable ...$iterables? That's the syntax PHP uses.

src/iter.php Outdated
*
* @return \Iterator
* @return iterable
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we should guarantee that the return value is Iterator, rather than any iterable. That would make the laziness of these functions (somewhat) part of the signature contract.

src/iter.php Outdated
@@ -869,11 +837,11 @@ function join($separator, $iterable) {
* iter\count(iter\flatten([1, 2, 3, [4, [[[5, 6], 7]]], 8]))
* => 8
*
* @param array|Traversable|\Countable $iterable The iterable to count
* @param iterable|array|Traversable|\Countable $iterable The iterable to count
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array and Traversable can be dropped here, as they are covered by the iterable now.

@enleur
Copy link
Contributor Author

enleur commented Dec 25, 2017

Done

*
* @return \Iterator
*/
function map(callable $function, $iterable) {
_assertIterable($iterable, 'Second argument');
function map(callable $function, iterable $iterable): iterable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @return annotation and the "actual" return type are different now, should also be \Iterable in the return type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\Iterable or just iterable?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Urgh sorry, that was a typo, I was referring to \Iterator there.

src/iter.php Outdated
@@ -514,7 +497,7 @@ function drop($num, $iterable) {
*
* @return \Iterator
*/
function repeat($value, $num = INF) {
function repeat($value, int $num = INF): iterable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://3v4l.org/cgnkk this is going to throw an Error when called without the second argument. Looks like there is no test for that case, so this didn't turn up in CI :(

src/iter.php Outdated
function slice($iterable, $start, $length = INF) {
_assertIterable($iterable, 'First argument');

function slice(iterable $iterable, $start, $length = INF): iterable {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $start here can also be int (though $length can't be).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it use PHP_INT_MAX instead?

@enleur
Copy link
Contributor Author

enleur commented Dec 25, 2017

I'm a bit confused about the right return types in phpdoc blocks. Should it be iterable, \Iterable or \Iterator?

@nikic
Copy link
Owner

nikic commented Dec 25, 2017

The right return type is one of iterable, \Traversable, \Iterator or \Generator, in order of increasing specificity. I prefer \Iterator in this case, because it guarantees the availability of the usual iterator methods, while not exposing the generator specific functionality (which is not used/supported here).

@enleur
Copy link
Contributor Author

enleur commented Dec 29, 2017

@nikic fixed

@nikic
Copy link
Owner

nikic commented Jan 2, 2018

Merged as c40a7ec, thanks!

@nikic nikic closed this Jan 2, 2018
@jvasseur jvasseur mentioned this pull request Mar 19, 2019
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

Successfully merging this pull request may close these issues.

3 participants