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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull request checklist intent is unclear #1609

Open
lassoan opened this issue Feb 12, 2020 · 7 comments 路 May be fixed by #1634
Open

Pull request checklist intent is unclear #1609

lassoan opened this issue Feb 12, 2020 · 7 comments 路 May be fixed by #1634
Assignees

Comments

@lassoan
Copy link

@lassoan lassoan commented Feb 12, 2020

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

This comment has been minimized.

Copy link
Member

@dzenanz dzenanz commented Feb 12, 2020

The Python-wrapping point could be rephrased as:

  • Python wrapping is added for any new classes
@dzenanz

This comment has been minimized.

Copy link
Member

@dzenanz dzenanz commented Feb 12, 2020

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

@hjmjohnson

This comment has been minimized.

Copy link
Member

@hjmjohnson hjmjohnson commented Feb 12, 2020

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

@lassoan

This comment has been minimized.

Copy link
Author

@lassoan lassoan commented Feb 12, 2020

"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

This comment has been minimized.

Copy link
Member

@jhlegarreta jhlegarreta commented Feb 12, 2020

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:
453f1e9
9ee19c3

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

This comment has been minimized.

Copy link
Member

@thewtex thewtex commented Feb 18, 2020

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

This comment has been minimized.

Copy link
Author

@lassoan lassoan commented Feb 18, 2020

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)
@dzenanz dzenanz linked a pull request that will close this issue Feb 18, 2020
7 of 7 tasks complete
@dzenanz dzenanz assigned dzenanz and unassigned jhlegarreta Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can鈥檛 perform that action at this time.