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

Include common review comments in the PR template #351

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Include common review comments in the PR template #351

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0f7680
Include common review comments in the PR template
BrieucF Jul 5, 2024
6cff611
Fix typo in PR template
BrieucF Jul 5, 2024
b53d4ee
Fix typo in PR template
BrieucF Jul 5, 2024
7e98799
encapsulate recommendations in comment
BrieucF Jul 5, 2024
83c496c
Add a reminder to check for overlaps
BrieucF Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the XML comment signs?

Change everything into checkboxes?

To ease the review process, please consider the following before to open the pull request:
- is your code well commented?
- did you update the relevant README(s)? See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
- if you added some code, is it covered by tests? See https://github.com/key4hep/k4geo/blob/main/lcgeoTests/CMakeLists.txt
- is your PR branch up to date with origin 'main'?
- are you not adding/modifying files that should not have been added/modified (or at least, not in this PR)?
- if you added/modified a detector geometry, have you checked that was no overlap? You can do so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" >> overlapDump.txt
andresailer marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +2 to +8
Copy link
Contributor

@andresailer andresailer Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
To ease the review process, please consider the following before to open the pull request:
- is your code well commented?
- did you update the relevant README(s)? See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
- if you added some code, is it covered by tests? See https://github.com/key4hep/k4geo/blob/main/lcgeoTests/CMakeLists.txt
- is your PR branch up to date with origin 'main'?
- are you not adding/modifying files that should not have been added/modified (or at least, not in this PR)?
- if you added/modified a detector geometry, have you checked that was no overlap? You can do so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" >> overlapDump.txt
To ease the review process, please consider the following before to open the pull request:
- [ ] the code is sufficiently well documented
- [ ] the relevant README(s) were updated. See e.g. https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/README.md or https://github.com/key4hep/k4geo/blob/main/FCCee/IDEA/compact/README.md
- [ ] the code is covered by tests. See https://github.com/key4hep/k4geo/blob/main/test/CMakeLists.txt
- [ ] the PR source branch has been rebased (`--ff-only`) to `k4geo/main`
- [ ] the PR does not contain any additions or modifications that do not belong to it
- [ ] the changes in this PR have not introduced any overlaps. You can do so with the following command: ddsim --compactFile PATH_TO_COMPACT_FILE --runType run --ui.commandsInitialize "/geometry/test/run" >> overlapDump.txt


If your modifications are compliant with the above, populate the release note text below, and open the PR!
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If your modifications are compliant with the above, populate the release note text below, and open the PR!
- [ ] The release notes below contain a succinct and comprehensive description of the changes that are sufficiently self-explanatory

-->

BEGINRELEASENOTES
- Thank you for writing the text to appear in the release notes. It will show up
exactly as it appears between the two bold lines
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exactly as it appears between the two bold lines
exactly as it appears between the two ALL CAPS lines

- ...

ENDRELEASENOTES
ENDRELEASENOTES

Loading