propagating alternative cause if it's not checked #945
Conversation
| @@ -17,7 +20,8 @@ | |||
| private final static Logger log = getLogger(ExecuteMethodExceptionHandler.class); | |||
|
|
|||
| public void handle(Exception exception) { | |||
| if (exception instanceof IllegalArgumentException) { | |||
| if (exception instanceof IllegalArgumentException | |||
| || exception instanceof IllegalStateException) { | |||
There was a problem hiding this comment.
I'm almost tending to use VRaptor specific exceptions for this case, instead of reusing IllegalArg and IllegalState...
We're doing this only because of validations, right?
We could create a ValidationFailedException.
|
Look good to me. I just didn't understand why we are wrapping |
|
about the |
|
guys, telling the truth, I've no idea why we wrap the does anyone know why we're doint it? (and looking here, VRaptor 3 wrap it too) |
|
If there is no reason, can't we just create the new exception as you guys suggested? |
|
yes, if truly there is no reason (I guess, not sure), we can refactor it removing |
|
👍 |
|
I don't remember the reason, but I see no harm in doing it. I just looked at the file history on VRaptor 3, and these catches went back to early 2009, first versions of VRaptor. Remove it, and we let people test it and see if some bugs appear, but I don't see why it would happen. |
|
:happy_panda: |
|
I've updated my PR. What do you think? |
|
👍 |
|
🚢 |
propagating alternative cause if it's not checked
|
😍 |
As suggested by @lucascs here. Closes #911.
It breaks compatibility, but we're going to
4.2and it'll be very highlightedon release notes. I'll add some docs before merging it. What do you think?