New Exceptions System #60
Conversation
Rebuild exception system
|
Hi @julien-boudry, I've now pushed a few new exceptions to this branch. It would be great if you could take a look and tell me if the code is fine with you. It would be a pity if I'm running in the wrong direction. 😄 Regards, |
Hi @toddy15
Thx for all. |
|
Another problem with the tests: We cannot know if the coverage is correct on the real tests, because we test each class even if not used in the real world (or lack of real tests). Instead, I propose to eliminate these unnecessary tests. And to write them only if there were special cases to test that are not satisfied in the functional tests, or if there are features specific to certain Exception classes, which is not the case here, they do not have their own code. |
|
Have a look on f357935 This should be applied to the others and the following ones. |
julien-boudry
force-pushed
the
WORK/newExceptions
branch
from
9285652
to
f357935
Compare
Both classes are in the same namespace, so the use statement is superfluous.
|
Hi @julien-boudry, thanks for your input. Yes, you're right, the new tests I've added provide very little confidence. Basically, they just assure that the exception classes are instantiable and of the correct type. However, the functional tests give more confidence. I'm fine with deleting all those new tests and add some more 'asserts' to the functional tests. So your commit f357935 is alright with me. About adding the abstract class CondorcetPublicApiException: I like that a lot, and in fact, I did think about this already before you've mentioned it. I thought that it might make sense to convert the previous "CondorcetException" to the new exception system and then, when there are no "exception codes" left in CondorcetException, use that as the base class. This way, code using the library would not have to be rewritten for v3.2.0, because they still can catch the generic "CondorcetException". On the other hand, it might be better with the new class you've introduced, because it's a clear break for downstream projects, so hopefully there won't be silent bugs because the downstream author still relies on the old exception system. Hm, not quite sure about the best approach here. |
|
Hi @toddy15, Yes, it's easy with Docker, I do it myself with shared volume, directly from official PHP images. I will do the merge from dev-3.2 branch (wich have merged few hours ago with php 8.1) into newExceptions branch. |
# Conflicts: # Tests/lib/Algo/Methods/STV/SingleTransferableVoteTest.php # Tests/lib/CandidateTest.php # Tests/lib/ElectionTest.php # Tests/lib/VoteTest.php # lib/Algo/Methods/CondorcetBasic.php # lib/Throwable/CondorcetException.php
…et into WORK/newExceptions
|
Merged! |
|
Great, thanks! The branch is now almost ready to be merged into dev-3.2, there are only a couple of minor thing to fix. In some places of the code, the CondorcetException is still used. |
Yes, the last ones are harder to fix, because maybe some of them need some new tests (actually, only coverage is lower). For example with Vote.php. |
|
Thanks for all! I will see the problem with the Vote class tomorrow. |
|
Soo ... I think that I've spotted every left over occurence of CondorcetException now, apart from some places in Documentation -- but as I understand it, you generate those files, right? So from my perspective, this branch could be merged into dev-3.2. |
|
Very nice job @toddy15! If you wants to continue to contribute, you can have a look at issues (or submit any ideas): |
|
Hi @julien-boudry, thanks! Indeed, I'd be interested in looking into the infection/infection implementation (#53). Apart from that, have you heard of https://pestphp.com/? It seems to be a really nice testing framework, capable of running the standard PHPUnit tests. However, the tests are much nicer to read and write -- at least in my opinion. |
|
Let's continue in the discussion section. And a new pull request for Infection. |
No description provided.