Add NoctudCollectionException and SequenceLogicException - #42
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
delacry
left a comment
There was a problem hiding this comment.
Thanks, btw when we are here, I found that we have @throws OutOfBoundsException on removeAt() methods, but in fact we are throwing Noctud\Collection\Exception\IndexOutOfBoundsException, I'll fix it in another PR, because that's 0.1.x bug
also we are missing @throws docblock in Collection::zip() which can throw NonReplayableSourceException in 0.2.x, maybe worth adding in this PR
be5fd46 to
b173e33
Compare
Every exception this library raises now implements NoctudCollectionException, so a single catch covers them all. An interface rather than a base class: each failure keeps the parent that describes it best, and a third-party collection built on this library can join the family without giving up the exception hierarchy it already has. SequenceLogicException extends it and marks the two failures any Sequence method can raise - a source that refuses to hand back a fresh pass, or one that hands back something not iterable. Which one surfaces depends on the source, not on the method called, so the two @throws lines repeated on all 41 Sequence methods collapse into a single one declaring that type. Refs noctud#23
The exception can also be thrown from Collection::zip(), so the Sequence prefix was misleading.
b173e33 to
04be273
Compare
|
@delacry PHPStan is failing here, but it is failing on 0.2.x, not due to this PR |
delacry
left a comment
There was a problem hiding this comment.
Yeah, I'll fix it separately on 0.2.x, probably some new version of PHPStan flagged some older tests
btw could you also add the @throws into Collection::zip() docblock?
| * Marks the failures any Sequence method can raise, whatever that method does: a source | ||
| * that refuses to hand back a fresh pass, or one that hands back something not iterable. | ||
| * | ||
| * Every terminal operation declares this single type rather than the concrete exceptions | ||
| * behind it - which one surfaces depends on the source, not on the method called. |
There was a problem hiding this comment.
| * Marks the failures any Sequence method can raise, whatever that method does: a source | |
| * that refuses to hand back a fresh pass, or one that hands back something not iterable. | |
| * | |
| * Every terminal operation declares this single type rather than the concrete exceptions | |
| * behind it - which one surfaces depends on the source, not on the method called. | |
| * Marks the failures of an iterable handed to the library that cannot be walked (again): | |
| * a source that refuses to hand back a fresh pass, or one that hands back something not | |
| * iterable. Any Sequence terminal operation can raise it, and so can Collection::zip() | |
| * when the iterable passed to it cannot be rewound. | |
| * | |
| * Sequence's terminal operations declare this single type rather than the concrete | |
| * exceptions behind it - which one surfaces depends on the source, not on the method called. |
|
I've fixed the PHPStan issues, so you can do a rebase of failing branches.. one issue turned out to be some regression (phpstan/phpstan#15321) so I've locked it down to 2.2.15 for now |
Collection::zip() rewinds the other iterable eagerly, so an iterator whose rewind() fails surfaces there. SourceException no longer presents itself as Sequence-only, since walking any source can raise it.
Follow-up from #23 (checklist, Follow-ups), where the review thread on #31 noted that every
Sequencemethod declares the same couple of exceptions, and that a single library-wide parent would let one try/catch cover everything.Two marker interfaces
NoctudCollectionExceptionis implemented by every exception the library raises:An interface rather than a base class, for two reasons: each failure keeps the parent that describes it best —
NonReplayableSourceExceptionstays anUnsupportedOperationException, which is also whatCollection::zip()raises outside of any sequence — and a third-party collection built on this library can join the family without giving up the exception hierarchy it already has.What it buys on
SequenceNonReplayableSourceExceptionandInvalidSequenceSourceExceptionboth implementSequenceLogicException. Which of the two surfaces depends on the source, never on the method called, so declaring both on each of the 41 methods repeated the same two lines 41 times:The concrete types are still named where they carry per-case information: the
Sequenceinterface header, which says which source kind raises which, andSequenceLogicException's own docblock.Naming
NoctudCollectionExceptionrather thanCollectionLogicException: the latter reads as "a logic error about aCollection", which would suggestSequencefailures are not part of it. The package name carries no such ambiguity.Tests
tests/ExceptionHierarchyTest.php, four cases proving both markers catch what they claim: an eager failure, and both sequence source failures.