Use zip for iterating over the formatted values

[okapies]

Fix to use zip for iterating over the formatted values. It prevents retrieving formatted_value by index especially when it is Iterable but not Sequence.

Ref. #204.

[aminalaee]

I think we could also update is_iterable to isinstance(x, Sequence) or isinstance(x, InstrumentedList).
The main reason we switched from Sequence to Iterable was that SQLAlchemy InstrumentedList was not a Sequence and now the Iterable conflict with strings is making it more complicated.

Now this change is not very explicitly showing the root-cause that we're dealing with strings, I think we need to fix in the right place.

[okapies]

By definition, Sequence is a trait both of Reversible and Collection with __getitem__, index and count. In this sense, InstrumentedList is actually a Sequence even if it is not a subclass of it. And thanks to abc module, you don't need to check if the value is an InstrumentedList apart from inspecting it is a Sequence:

>>> from collections.abc import Sequence
>>> from sqlalchemy.orm.collections import InstrumentedList
>>> isinstance(InstrumentedList(), Sequence)
True

But, choosing the minimum interface for treating value is generally good. If we just need to iterate over the value to render, we can use Iterable not Sequence. It also gives a lot of flexibility to the users when implementing format_*.

On the other hand, if we plan to use __getitem__, __contains__ and __len__ (i.e. random access functionalities) in our implementation in the future, we should choose Sequence. It depends on the requirement for the design of this library.

[okapies]

Note that str is also a Sequence, so we still need to check if it is not a string or bytes.

>>> from collections.abc import Sequence
>>> isinstance("foo", Sequence)
True

[okapies]

Anyway, I think merging this PR will not affect your design decision because Iterable (accepted by zip) is less strict than Sequence.

[aminalaee]

@okapies Yes you're right about the definitions, my comment was not really accurate.
Previous instead of is_iterable it was is_list and we were checking isinstance(x, list) but because there was an issue distinguishing SQLAlchemy InstrumentedList we exactly switched to abc Sequence and Iterable.

But since isinstance(InstrumentedList(), list) == True then I'm not sure why it should be is_iterable when it can be is_list simply. Let me dig a bit into the older PRs.

[aminalaee]

I know this won't discard your PR, using zip would still make it more readable in template, it will just make the type checking of is_iterable or is_list strict.

[okapies]

Ok. Anyway, could you please release a new version including #204? It is a show-stopper for our app.

[aminalaee]

Ok. Anyway, could you please release a new version including #204? It is a show-stopper for our app.

Yes sure, I'll merge these and do the release.

[aminalaee]

Thanks for the PR 👍

[okapies]

🙇