Itk: Pull request checklist intent is unclear

[lassoan]

Description

I've submitted a pull request and met with a checklist that I couldn't figure out who should fill and use and how:

🚫 Makes breaking changes
🚫 Makes design changes
🚫 Adds the License notice to new files.
🚫 Adds Python wrapping.
🚫 Adds tests and baseline comparison (quantitative).
🚫 Adds test data.
🚫 Adds Examples to ITKExamples
🚫 Adds Documentation.

What the red "no_entry_sign" means? Is it bad if checked or bad if unchecked?

I can see how "makes breaking changes" or "design changes" are red flags that developers should self-declare (although this can be checked very easily by the reviewers, too). So, checked means something bad (or at least requires attention).

"Breaking change" is also very vague. Is fixing a bug a breaking change? (if an application relied on the earlier, incorrect behavior, would you consider it a breaking change?)

"Adds the License notice to new files" this time , checked means OK

"Adds Python wrapping" - I don't understand this at all. I would expect that Python wrapping is automatic and transparent. Maybe you mean changed API? Or that Python wrapping has to be tested?

"Adds tests and baseline comparison (quantitative)." and "Adds test data." are so tightly linked that it should be one item.

It would also make the checklist more clear if the items were simple past tense.

Something like this would be more clear:

• [ ] API did not change (or the change has been approved)
• [ ] No major design change was made (or the change has been approved)
• [ ] If new files were added, license notice was added
• [ ] Added test
• [ ] Added example
• [ ] Updated API documentation (or API did not change)

The PR could be merged if all items are checked.

[dzenanz]

The Python-wrapping point could be rephrased as:

• [ ] Python wrapping is added for any new classes

[dzenanz]

The suggested change sounds like an improvement.
@blowekamp @hjmjohnson @thewtex

[hjmjohnson]

I agree that these are not easy to interpret. Simplifying would make the processes less confusing.

[lassoan]

"Python wrapping is added for any new classes" - yes, this is clear, we just need to add a link to the documentation that describes how to do this.

Updated version:

• [ ] No API changes were made (or the changes have been approved)
• [ ] No major design changes were made (or the changes have been approved)
• [ ] Added test (or behavior not changed)
• [ ] Updated API documentation (or API not changed)
• [ ] Added license to new files (if any)
• [ ] Added Python wrapping to new files link TBD (if any)
• [ ] Added to ITK examples for all new major features (if any)

[jhlegarreta]

I also share that feeling to a certain extent and admit that the checkbox list may be subject to interpretation: it may be used differently by contributors, and thus may not be used or be useful at all in that case.

Just to provide a background, these were added and modified by these two commits:
https://github.com/InsightSoftwareConsortium/ITK/commit/453f1e9a9c376462bac6a9a8fb25c573005065f2
https://github.com/InsightSoftwareConsortium/ITK/commit/9ee19c3b5e78d532977c5222de350b1f1126e691

The idea of using checkboxes was, among others, to allow reviewers spot more easily and especially PRs that may imply changes that may impact the toolkit to an extent which needs special attention. It is not exclusive of ITK.

I admit that even in the best case, since they depend on a human checking them, at times they may not honor the changes of the PR.

Since I am under a heavy workload now, I would allow myself some more time, meaning at least a month, to think about them and propose a change. Otherwise, if you reach consensus faster, please, feel free to move forward and merge.

[thewtex]

Added Python wrapping to new files link TBD

This can be:

Added Python wrapping (Book 1 Section 9.5) to new files (if any)

Added license to new files (if any)

We can add a link here, too.

Added license to new files (if any)

[lassoan]

Thanks! Updated version:

• [ ] No API changes were made (or the changes have been approved)
• [ ] No major design changes were made (or the changes have been approved)
• [ ] Added test (or behavior not changed)
• [ ] Updated API documentation (or API not changed)
• [ ] Added license to new files (if any)
• [ ] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
• [ ] Added to ITK examples for all new major features (if any)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.