ORM: replace InputValidationError with ValueError and TypeError
#4888
Conversation
No, as discussed during the meeting, the exceptions still have their use (they are still used in the |
Just read through the related issue. Indeed, you're right. Looking through the doc strings of the exceptions as well, I think |
I mean you could, I just don't see the point. Who would be catching these exceptions? And when you want them to be caught, unless there is a specific reason they need to be very distinguishable, I think custom exceptions often make things more complicated than necessary. |
I quite disagree. As long as you keep a proper hierarchy it's quite useful to have explanatory exception names in the code. For me it's not so much about the "catchability" (for this, a proper hierarchy and a parent exception can be used), but for more readable code, quickly making it clear why someone thinks it's necessary (what the intention is) to raise an exception in the code. But that's just me :) But for the first part, I would update the doc strings of the exceptions to make it clear where it should be used then, to avoid similar things happening in the future. Although, it can be argued in this case that it doesn't matter, since the doc string was already quite explicit for |
sphuber
force-pushed
the
fix/3812/orm-replace-input-validation-error
branch
from
cf2c39c
to
3e5b39b
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4888 +/- ##
===========================================
- Coverage 80.08% 80.07% -0.00%
===========================================
Files 518 518
Lines 36680 36671 -9
===========================================
- Hits 29371 29361 -10
- Misses 7309 7310 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@ltalirz could you please follow up on the discussion in the issue of this PR. And maybe some one else still wants to review this? @chrisjsewell @ramirezfranciscof ? |
There was a problem hiding this comment.
Thanks @sphuber ! I have a couple of small things (some may be nitpicks) to mention. One of the comments though might be worth generalizing: since you are adapting all of these raises, would it make sense to turn most (if not all) of them into re-raises? (so, except ... as ... and raise ... from ...)
aiida/tools/graph/age_rules.py
Outdated
| @@ -208,7 +207,7 @@ def _init_run(self, operational_set): | |||
| for proj_tag, projectionlist in projections.items(): | |||
| try: | |||
| self._querybuilder.add_projection(proj_tag, projectionlist) | |||
| except InputValidationError: | |||
| except ValueError: | |||
There was a problem hiding this comment.
add_projection can also raise TypeError, right? (line 1180 of aiida.orm.querybuilder or whereabouts) Are you not catching that one because there is no point in adding the extra info*?
Btw, shouldn't we except ... as ... and raise ... from ... here?
(EDIT) * Or also, changing the raise to a KeyError...
There was a problem hiding this comment.
No reason, you are right, should also catch the TypeError.
There was a problem hiding this comment.
Btw, since I think you wrote this code: why do you raise a KeyError here and why has the string an explicit newline?
There was a problem hiding this comment.
Yes, now I don't remember if I added this or it was originally like that on the base AGE branch; in any case I can't think of any good reason so I was probably just blundering 😅
If you want to change it though, is there a way to re-raise the same kind of exception? (since you are catching both ValueException and TypeException, you don't know a-priori which one to re-raise...)
sphuber
force-pushed
the
fix/3812/orm-replace-input-validation-error
branch
from
3e5b39b
to
2cee3cd
Compare
There was a problem hiding this comment.
I noticed a couple more of raise ... from ... that I think could be added; you can check them out but other than that this is ready!
The only other thing I noticed is that there seem to be a lot of validation pathways that are not being unit-tested. I was going to propose to open an issue because I thought this was mostly for Querybuilder error handling, but I see this is also in the data plugins so I'm not sure. Adding one issue for each or one issue for all seem both not ideal, although if I had to choose I would probably go for the latter, no?
The front-end ORM code threw an `InputValidationError` in a number a places. However, this exception was intended for calculation plugins to throw in the case of invalid input nodes. Where it was being used in the ORM plain `ValueError` or `TypeError` exceptions should have been used. Since the `InputValidationError` does not subclass either of the base exception types, code that was catching this particular AiiDA-specific exception will break after these changes. However, a scan of the most used plugins revealed that this exception is actually not being caught so this change should have little to no actual consequences. In addition, the `CodeValidationError` custom exception of the class `aiida.orm.utils.builders.code.CodeBuilder` was changed to subclass the `ValueError` instead of the base `Exception`.
sphuber
force-pushed
the
fix/3812/orm-replace-input-validation-error
branch
from
2cee3cd
to
84ac26b
Compare
I think one for the |
|
I have found nothing with the tags refactoring, testing or querybuilder. I even went through the whole list of a simple "test" search and nothing. I also had the idea that there was one, but if it is there is very well camouflaged =S. Should I create two issues then, one for general coverage and one for the querybuilder in particular? |
|
Sure, if you can't find anything, go ahead and open an issue for it. How it should be split depends really. We had one single issue before about test coverage that @ltalirz opened and then closed once the biggest offenders were fixed. Maybe we need to reinstate it or create a new one where you add these new offenders |
Fixes #3812
The front-end ORM code threw an
InputValidationErrorin a number aplaces. However, this exception was intended for calculation plugins to
throw in the case of invalid input nodes. Where it was being used in the
ORM plain
ValueErrororTypeErrorexceptions should have been used.Since the
InputValidationErrordoes not subclass either of the baseexception types, code that was catching this particular AiiDA-specific
exception will break after these changes. However, a scan of the most
used plugins revealed that this exception is actually not being caught
so this change should have little to no actual consequences.