search

chaijs / chai

BDD / TDD assertion framework for node.js and the browser that can be paired with any testing framework.
https://chaijs.github.io
MIT License
8.11k stars 694 forks source link

Implement `iterable` assertion #1592

Closed koddsson closed 6 months ago

koddsson commented 7 months ago

Pulled this out of #1583 so that these PRs make more logical sense.

This PR implemensts a iterable/isIterable assertion that checks a given object for Symbol.iterator but notably excludes string which does have Symbol.iterator but we don't consider to be "iterable".

koddsson commented 7 months ago

I’m curious why explicitly disallow string for this check?

You know, I don't remember why I put that in there. I have a vague memory of the sameMembers test needing this but looking at this in isolations makes me think that's probably a oversight.

koddsson commented 7 months ago

Is this bit meant to be part of this PR?

Not sure if GitHub is glitching but I can't see what this is referring to. I assume it's the isSubsetOf changes in which case, it shouldn't have been included and I've removed those changes. Thanks :)

43081j commented 7 months ago

I think this is good but I am still a little uneasy about overloading .an() too much.

curious, do you prefer .is.iterable then? just so we're all on the same page for other changes in future

im guessing you're unsure of leaning heavily on an for many things, since over time chai would mostly be that one chain

keithamus commented 7 months ago

I'd prefer each assertion (especially core assertions) to map to something that a user can reproduce with a single expression. It makes it far easier for us to document, and for users to internalise. If they know .an() is just Chai's shorthand for [Symbol.toStringTag] = then that's an easier way to think about it than having a mental lookup table to some alternative logic.

43081j commented 7 months ago

makes sense to me 👍

thanks for the explanation, i do agree

koddsson commented 7 months ago

I think this is good but I am still a little uneasy about overloading .an() too much.

I'm happy to change this. I'd rather nip this in the bud now than trying to untangle it later.