-
Notifications
You must be signed in to change notification settings - Fork 71
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
Small improvements and fixes #800
Conversation
src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionExecutionListenerTests.java
Show resolved
Hide resolved
Hey, thanks for the PR, but you know that we decided to have an issue for each PR. For me it's okay to create a "replace Junit asserts for AssertJ ones" issue, even if you do slightly more in some places. |
We have #790 |
If you would have proposed a describing message in your intro post and linked the issue I would have known ;) I guess we can take this as a base for this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the indeed small improvements! I have a few more questions (see comments).
As a small side note: various independent changes are made within a (large) commit. We also do this when squashing PR commits, but usually alongside a meaningful commit message and an issue link. As a reviewer, I find some changes in this PR not comprehensible, at least at first sight. It would be helpful to have smaller, atomic, and semantically coherent commits with appropriate messages. ✌️
@@ -310,7 +310,7 @@ tasks { | |||
javadoc { | |||
if (releaseBuild) { | |||
javadocTool.set(project.javaToolchains.javadocToolFor { | |||
// create Javadoc with least Java version to get all features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"latest"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this means that we use the smallest Java version we need for all the features we want to use but it's phrased a bit weird?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe something like:
// create Javadoc with the minimum Java version needed for our desired features (e.g. search)
src/demo/java/org/junitpioneer/jupiter/DisableIfTestFailsExtensionDemo.java
Show resolved
Hide resolved
src/demo/java/org/junitpioneer/jupiter/cartesian/CartesianTestExtensionDemo.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/ExpectedToFailExtensionTests.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/issue/IssueExtensionExecutionListenerTests.java
Show resolved
Hide resolved
src/test/java/org/junitpioneer/jupiter/resource/ResourcesTests.java
Outdated
Show resolved
Hide resolved
…into issue/790-maintenance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One optional comment left. Feel free to merge. ✌️
@@ -310,7 +310,7 @@ tasks { | |||
javadoc { | |||
if (releaseBuild) { | |||
javadocTool.set(project.javaToolchains.javadocToolFor { | |||
// create Javadoc with least Java version to get all features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe something like:
// create Javadoc with the minimum Java version needed for our desired features (e.g. search)
Closes #790
Proposed commit message:
PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.
Documentation (general)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
file references demo insrc/demo/java
instead of containing code blocks as text.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode (general)
Code (new package)
module-info.java
module-info.java
Contributing
README.adoc
mentions the new contribution (real name optional)