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 idiomatic approach for is_iterable method #215

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use idiomatic approach for is_iterable method #215

wants to merge 2 commits into from

Conversation

askoretskiy
Copy link

Use idiomatic Python code instead of try-except.

That also makes iterutils consistent -- some methods used isinstance(obj, Iterable) and some used try-except.

Also for common methods (is_scalar and is_collection) use code directly, without is_iterable.

@askoretskiy
Copy link
Author

@mahmoud it seems AppVeyor build is broken (not by my changes :-)

@mahmoud
Copy link
Owner

mahmoud commented Jun 21, 2019

Haha, dangi Appveyor, I'll look into that. But first things first.

What would you say makes this idiom better than the other. They're certainly both idioms.

One reason I went with this route instead of checking the abstract base class was that isinstance checks can be quite slow on inherited classes. Also, I like the directness of this check, whereas I'm pretty sure the try/except route provides more practical safety. I could construct a scenario where the try/except returns False and the isinstance returns True.

@askoretskiy
Copy link
Author

If you check Iterable source code, you'd find that Python developers are overriding the logic that stands behind isinstance. I suppose that is the RIGHT way to check if variable is iterable or not. I suppose Python developers extracted exactly logic of the method is_iterable into this method as reference implementation. I'd call this method idiomatic.

Second issue is that some methods in iterutils are not consistent. Some are using isinstance(obj, Iterable), other are using is_iterable. If isinstance(obj, Iterable) is giving different results than is_iterable, why is it used? Then all parts of the code should use is_iterable. If they are equal, we should do other way around -- all the parts should use isinstance(obj, Iterable). I just would like to have consistency here.

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.

2 participants