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 is_iterable instead of is_array #991

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Conversation

alexgt9
Copy link
Contributor

@alexgt9 alexgt9 commented Mar 20, 2020

What about using is_iterable to allow Traversable objects to be processed automatically?

If the check is only to be able to do a foreach, I think it's safe.

@alexgt9
Copy link
Contributor Author

alexgt9 commented Mar 20, 2020

If you think that this change deserve it's existence I can add tests

@ste93cry
Copy link
Contributor

Yes please, once the tests are in place I will gladly merge the PR

@alexgt9
Copy link
Contributor Author

alexgt9 commented Mar 23, 2020

@ste93cry I just committed the test 👍

@alexgt9 alexgt9 force-pushed the patch-1 branch 2 times, most recently from fa23cf4 to 12b67c8 Compare March 23, 2020 17:44
@ste93cry
Copy link
Contributor

Instead of merging, can you please rebase on top of develop and consequently change the target branch to it?

@alexgt9 alexgt9 changed the base branch from master to develop March 23, 2020 17:49
@alexgt9
Copy link
Contributor Author

alexgt9 commented Mar 23, 2020

Sure, I just did it, I didn't know you were using develop

Copy link
Contributor

@ste93cry ste93cry left a comment

Choose a reason for hiding this comment

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

Thank you, changes look good, LGTM

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

We're missing a changelog entry! Apart from that, LGTM! 👍

@alexgt9 alexgt9 force-pushed the patch-1 branch 2 times, most recently from 8abdff8 to 43f1c48 Compare March 24, 2020 08:42
@alexgt9
Copy link
Contributor Author

alexgt9 commented Mar 24, 2020

We're missing a changelog entry! Apart from that, LGTM! 👍

Ouch!, added.

@guilliamxavier
Copy link
Contributor

I'm sorry but this seems to break Behat, please see #1028

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

Successfully merging this pull request may close these issues.

4 participants