-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@StandardException annotation for generating exception constructors #2702
Conversation
I thought this would have been part of the new 1.18.18 version but don't see it in there.... When will |
I like it, but we're on the hot seat to push 1.18.20 out the door. Only real hold up is a thorough review. @rspilker - what are you thoughts on the feature itself? TODO:
|
* annotation with javac and Eclipse handlers * single test file
530ca07
to
af16ba2
Compare
* move feature under experimental * replace ProviderFor with Provides * add doc material (to be completed) * add author
@rzwitserloot treating this as experimental seems fair, so I moved it. I also rebased, added my name and started documentation. |
Reviewing the basic feature idea itself, I see some problems. Some exceptions simply do not have the 'set of 4', so implementing all 4 constructors as, respectively:
will fail for these. Admittedly a lot of exceptions are growing this 'standard set', but there are still plenty that just do not have them. For example, I'm not quite sure why you'd want to extend that class, but you can (it is not A second issue is that you may want to do stuff in addition to the standard super call. By implementing the set-of-4 with a We can fix both of these problems in one fell swoop but that brings in new problems. Proposal: The set-of-4 is implemented as follows (so, this is the delomboked version of
This has considerable advantages:
It also has disadvantages:
I think in the grand scheme of things this is the best available option, but I could be convinced there are better alternatives. Thoughts? |
Also, there is room for an expansion to the feature (perhaps better left as a second milestone and not for a first release): What if we make it possible to annotate a constructor in a class with This would let you make constructors that take additional arguments. Admittedly vastly more exotic than the usual 'just gimme the standard batch of 4 please' strategy. |
I've reviewed and implemented the above changes (but not the 'annotate an existing constructor' suggestion). @rspilker still needs to review the feature, though. I'll try my best to make the case for this feature :) |
@ttzn I had to refactor quite a bit but please don't take that as an indicator of quality of the PR - your PR was a welcome sight and helped a ton! Thank you! |
One more update I'd like to put in: What should lombok do if the annotated class extends nothing at all? E.g:
The two obvious actions are:
Ordinarily I always prefer 'do not presume to know what the developer wants if there are multiple plausible answers, and instead generate an error that explains precisely what needs to be done', but there is a case to be made that 'just extend Exception' is convenient, and it does mean you can just write Thoughts? |
To make this logic work, perhaps making a nearly equivelent I haven't tried to implement this, but I'm sure there is a way to do this with minimal repeated code. I suppose we could have
Where @StandardThrowable contains most of the logic and @StandardException or @StandardRuntimeException contain the specifics related to extending Exception or RuntimeException. With this in mind, a user can either extend Exception/RuntimeException or not extend Exception/RuntimeException, it'd be a redundant statement. |
@esend7881 If I would choose between having the different annotations, or having the author type an extends clause, the latter would always win. |
I agree, the user should manually specify the exception superclass. That actually makes more sense because a user could want to extend a specific exception such as IOException or IllegalArgumentException, etc. |
Can you please finally build and release Lombok with this feature? |
@martiench release is imminent. |
Hi, I have this code
At |
Fixes #1559, related to #375 and discussed in this thread.
This is a pretty advanced attempt at implementing the proposed
@StandardException
feature (naming to be discussed). It turns the following:... into:
If one of those 4 constructors is already declared, the handlers skip it. Naturally, compilation fails if any of the parent constructors is missing, as this is really meant to be used on subclasses of JDK throwables. Right now there is no configuration option to control the behaviour.
Before going further with cleanup and documentation, I'd like this to be thoroughly discussed. Some words on the implementation:
HandleConstructors
classes, with lots of cuts and modifications; some degree of factorization may be achieved by moving stuff into utility classes, which I plan to do if this feature is validated (along with a more comprehensive test suite);stringType
andthrowableType
in the javacSymtab
stub, as I found no way to reference these types using available APIs.I hope this proves useful. Thanks to the maintainers for all the hard work by the way.