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
Align use of extension point #496
Conversation
What's missing:
|
686ad4c
to
a7b7c28
Compare
@nipafx did a rebase, addressed the remaining tasks, and wrote a commit message. ✌️ |
The described behavior is indeed what we now offer but it's actually what could be expected from such annotations, so we don't need to document "the obvious" (cough). Instead, we should point out the change of behavior in the release notes.
Great work, thank you @beatngu13! I changed a few things - see commit messages. |
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.
I like it, especially the addition in the docs 👍
@beatngu13 will have a look in when he has a quite minute |
I think we wanted to model the tests of DefaulTimeZone after DefaultLocale before merging. |
At least the tests need to be aligned before this is merge-ready. Also, this conversation needs to be resolved. |
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.
Merge conflict needs to be resolved
I think I did it … proposed commit message:
|
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.
LGTM - I do have a couple nitpicks about it.
src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java
Outdated
Show resolved
Hide resolved
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, the build is platform-dependent now? How did that happen?
I forgot this: junit-pioneer/src/test/java/org/junitpioneer/jupiter/DefaultTimeZoneTests.java Lines 38 to 39 in 30cca6e
Apparently, the macOS job was using UTC, so the test default resulted in UTC+12. Fix: |
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.
See my comment on the comment-question, if I'm the only one who does it wrong, see this as approved.
Closes #479.
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
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.