Skip to content

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