Issues 938-document inheritance support#5708
Conversation
Signed-off-by: Swati Chauhan <swati.chauhan814@gmail.com>
swatichauhan814
force-pushed
the
issues/938-document-inheritance-support
branch
from
c4743d5 to
b9cd9a1
Compare
@jbduncan all yours. |
This comment has been minimized.
This comment has been minimized.
jbduncan
left a comment
jbduncan
left a comment
There was a problem hiding this comment.
This is a great start! I have some thoughts on how it could be improved further; see below.
@mpkorstanje, I was wondering if I could have your input on these too as a JUnit maintainer?
| * {@link ArgumentsAccessor} if an error occurs while accessing | ||
| * or converting an argument. | ||
| * | ||
| * <p>This class is not intended to be extended by clients. |
There was a problem hiding this comment.
Exceptions are a bit unusual. The base JUnitException is definitely designed for extension (as is RuntimeException for that matter), and I think it's fair to say that the JUnit maintainers may want to extend ArgumentAccessException in the future too, so we should give users the same right.
How about a header like this (inspired by Assertions.java) to make the wording on extension more prominent?
* <h2>Extensibility</h2>
*
* <p>This class is designed for extension.
@mpkorstanje, do you have any thoughts on all this?
There was a problem hiding this comment.
I think it's fair to say that the JUnit maintainers may want to extend ArgumentAccessException in the future too, so we should give users the same right.
As far as I can tell, the ArgumentAccessException shouldn't ever be created from user code. So this would have to be the "This class is not intended to be extended by clients." warning. But I'm not 100% sure about that.
In the user guide I see that users might implement the ArgumentsAccessor interface which would require them to be able to throw ArgumentAccessException.
https://docs.junit.org/6.1.0/writing-tests/parameterized-classes-and-tests.html#argument-aggregation
In this context, an indexed parameter is an argument for a given index in the Arguments provided by an ArgumentsProvider that is injected into a field annotated with
@Parameter(index). An aggregator is any@Parameter-annotated field of type ArgumentsAccessor or any field annotated with @AggregateWith.
But I'm not sure if the user guide is correct here, I couldn't find a code path that would make that possible and I think that instead of ArgumentsAccessor the guide should omit the type and say:
An aggregator is any
@Parameter-annotated field or any field annotated with @AggregateWith.
@jbduncan you could help me out here by double checking if this indeed the case.
Signed-off-by: Swati Chauhan <swati.chauhan814@gmail.com>
jbduncan
left a comment
jbduncan
left a comment
There was a problem hiding this comment.
@swatichauhan814 LGTM! Consider it approved on my end. Great work. 👍
I believe we need a review from a JUnit maintainer to get this merged in, so now we wait.
mpkorstanje
left a comment
mpkorstanje
left a comment
There was a problem hiding this comment.
Looks good!
@jbduncan one remark for you to look into.
3 participants
issue: Document which classes support inheritance and which do not #938
I hereby agree to the terms of the JUnit Contributor License Agreement.