Conversation
So make them checked and let compiler help you. Made checked 3 exceptions: * ModelBuilderEx * ProjectBuilderEx * ModelResolverEx And it immediately uncovered an avalanche of problems.
cstamas
changed the title
| throws E { | ||
| return ThrowingSupplierWrapper.process( | ||
| supplier, s -> cache.computeIfAbsent(groupId, artifactId, version, tag, s)); | ||
| } |
There was a problem hiding this comment.
This is exactly the kind of boiler plate code I wanted to avoid using runtime exceptions.
There was a problem hiding this comment.
most of this undone, we need to deal with one single exception only
There was a problem hiding this comment.
IMO, the latest push is quite sensible. Also, have to add "boilerplate code wanted to avoid" vs all the changes revealed just by the compiler, as exception went from unchecked to checked... which one is worth more for you?
|
I really don't think that works. What if the ModelBuilder calls internally a service which throws an unchecked exception ? If the caller catches ModelBuilderException (which he would have to because that one is checked), then it may miss the unchecked exceptions. I don't really see a good justification for having only a few ones being unchecked or checked. That could work if there were real differences, such as transient issues (as in downloading and a network error happens), but most exceptions here are not of this kind. |
cstamas
changed the title
|
Ok but @gnodet we need to pick up at least the bugfix from this PR, and consider maybe some others (you are catching ModelBuildingException while in fact ModelBuilderException is being thrown, etc). |
|
Removed all the fluff, now really only the proper placement of exceptions are done (and at least two bugfixes where runtime ex "escaped" causing "internal maven error"). @gnodet pls review |
2 participants
This PR merely cleans up exception usage across Maven.