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?
| * <p>Using this builder ensures consistency in how failure message are formatted | ||
| * within JUnit Jupiter and for custom user-defined assertions. | ||
| * | ||
| * <p>This class is not intended to be extended by clients. |
There was a problem hiding this comment.
Looks good. I think it would be even better with wording and a header like this (inspired by Assertions.java) to make things more prominent:
* <h2>Extensibility</h2>
*
* <p>Although it is technically possible to extend this class, extension is
* strongly discouraged.
| * {@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?
| * {@link ArgumentsAggregator} when an error occurs while aggregating | ||
| * arguments. | ||
| * | ||
| * <p>This class is not intended to be extended by clients. |
There was a problem hiding this comment.
Like my other comment on ArgumentAccessException, I think we should allow users to extend this class and we should use wording like this:
* <h2>Extensibility</h2>
*
* <p>This class is designed for extension.
| * {@link ArgumentsProvider} implementations that also need to consume an | ||
| * annotation in order to provide the arguments. | ||
| * | ||
| * <p>This class is designed for extension. |
There was a problem hiding this comment.
Looks good. Like my other comments, I think it would be even better with a header like this:
* <h2>Extensibility</h2>
*
* <p>This class is designed for extension.
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.
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.