Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Small improvements and fixes #800

Merged
merged 6 commits into from Mar 26, 2024
Merged

Small improvements and fixes #800

merged 6 commits into from Mar 26, 2024

Conversation

Michael1993
Copy link
Member

@Michael1993 Michael1993 commented Dec 27, 2023 •
edited

Closes #790

Proposed commit message:

${action} (${issues} / ${pull-request}) [max 70 characters]

${body} [max 70 characters per line]

${references}: ${issues}
PR: ${pull-request}

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)

  • There is documentation (Javadoc and site documentation; added or updated)
  • There is implementation information to describe why a non-obvious source code / solution got implemented
  • Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
  • Site documentation in .adoc file references demo in src/demo/java instead of containing code blocks as text
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

  • The docs/docs-nav.yml navigation has an entry for the new extension
  • The package-info.java contains information about the new extension

Code (general)

  • Code adheres to code style, naming conventions etc.
  • Successful tests cover all changes
  • There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
  • Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Code (new package)

  • The new package is exported in module-info.java
  • The new package is also present in the tests
  • The new package is opened for reflection to JUnit 5 in module-info.java
  • The new package is listed in the contribution guide

Contributing

  • A prepared commit message exists
  • The list of contributions inside README.adoc mentions the new contribution (real name optional)

@Michael1993 Michael1993 marked this pull request as ready for review December 28, 2023 00:03
@Michael1993 Michael1993 added the full-build Triggers full build suite on PR label Dec 28, 2023
@Bukama
Copy link
Member

Bukama commented Dec 28, 2023

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.

@Michael1993
Copy link
Member Author

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

@Bukama
Copy link
Member

Bukama commented Dec 28, 2023

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

beatngu13
beatngu13 reviewed Feb 19, 2024
View reviewed changes
Copy link
Member

@beatngu13 beatngu13 left a comment •
edited

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. ✌️

build.gradle.kts
@@ -310,7 +310,7 @@ tasks {
javadoc {
if (releaseBuild) {
javadocTool.set(project.javaToolchains.javadocToolFor {
// create Javadoc with least Java version to get all features
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"latest"?

Copy link
Member Author

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?

Copy link
Member

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)

build.gradle.kts Outdated Show resolved Hide resolved
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/DisableIfTestFailsTests.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
beatngu13
beatngu13 approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@beatngu13 beatngu13 left a 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. ✌️

build.gradle.kts
@@ -310,7 +310,7 @@ tasks {
javadoc {
if (releaseBuild) {
javadocTool.set(project.javaToolchains.javadocToolFor {
// create Javadoc with least Java version to get all features
Copy link
Member

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/test/java/org/junitpioneer/jupiter/ExpectedToFailExtensionTests.java Show resolved Hide resolved
@Michael1993 Michael1993 merged commit 1251593 into main Mar 26, 2024
33 checks passed
@Michael1993 Michael1993 deleted the issue/790-maintenance branch March 26, 2024 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bukama Bukama Bukama left review comments

@scordio scordio scordio left review comments

@beatngu13 beatngu13 beatngu13 approved these changes

@nipafx nipafx Awaiting requested review from nipafx

Assignees
No one assigned
Labels
full-build Triggers full build suite on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Polish demo code
4 participants
@Michael1993 @Bukama @beatngu13 @scordio