DOC: Add Create a Pull Request section with draft PR guidance#5975
DOC: Add Create a Pull Request section with draft PR guidance#5975blowekamp wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
Conversation
github-actions
bot
added
type:Documentation
|
Cool, @blowekamp Very glad to have such a guidance for PRs! 👍 Some more detailed comments coming up... |
| author has personally verified that: | ||
|
|
||
| - all automated CI tests pass, | ||
| - the implementation is correct and complete, |
There was a problem hiding this comment.
Can you please make this:
- the implementation is correct, complete and fully understood,
I think it's essential that we keep understanding the code, even when it is partially AI generated, to avoid cognitive debt. As I mentioned at https://discourse.itk.org/t/ai-generated-pull-requests-overwhelming-hard-to-review-carefully/7728/12?u=niels_dekker
There was a problem hiding this comment.
I am concerned that the new developer burden to meet all the requirements of contributing to ITK may discourage participation. I understand the intent of this request, but the tone may need to be more inviting to new developers.
There was a problem hiding this comment.
I understand the intent of this request, but the tone may need to be more inviting to new developers.
@hjmjohnson You mean like replacing words like "must" with more friendly and modest expressions like "please do..." or "we prefer to...", right?
| Reviewer time is a finite human resource. Do not request a review before these | ||
| conditions are met. |
There was a problem hiding this comment.
Please add something like:
When you request a reviewer to review your PR, allow the reviewer a
reasonable amount of time to do the review, before the PR is merged.
There was a problem hiding this comment.
"reasonable amount of time" should be changed to something concrete. "at least 1 working day for low impact changes (i.e., single test changes, documentation updates, changes, and longer times for more in-depth changes that impact core infrastructure, or fundamental algorithmic changes. "
|
|
||
| - all automated CI tests pass, | ||
| - the implementation is correct and complete, | ||
| - the PR description accurately reflects the changes made. |
There was a problem hiding this comment.
I think it would be very helpful if the AI prompt would be shared. Can we also add that to the guidelines? For example:
- the PR description includes a description of the AI prompt that was used to create the PR
There was a problem hiding this comment.
I like the desire for provenance, and I also tended to this desire this previously, but I no longer think "including the prompt" is reasonable or desirable. In current agentic engineering best practices, there are many prompts and agents features and settings and context management that go into patch development that are difficult to capture and burdensome to developer, and I do not think attempting to add this provides much value for reviewers in the end. It more noise. What matters is in the end when review a patch or looking back at the history of a patch is a concise, accurate, and descriptive commit message of what change was made and why it was made.
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
Good as-is. The feedback provided so far also looks useful.
5 participants
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.