Allow PR template to catch incomplete tasks

[cbkerr]

Final checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• The changes introduced by this pull request are covered by existing or newly introduced tests.
• The package documentation and framework documentation in signac-docs are up to date with these changes.
• I have updated the changelog and added any related issue and pull request numbers for future reference.

Description of bug fix

This makes all checkboxes required by adding an "or" to the ones that were previously under a separate heading of "if necessary".

test of relative link

I propose creating a label for possibly breaking changes.

This PR uses the new template before reviews

Motivation and Context

glotzerlab/signac-flow/pull/508 was merged with unchecked boxes. It will be easier to catch this if we don't use checkboxes as radio buttons or otherwise optional items.

[codecov]

Codecov Report

Merging #686 (68c0137) into master (a5937cb) will not change coverage.
The diff coverage is n/a.

❗ Current head 68c0137 differs from pull request most recent head 055f7c7. Consider uploading reports for the commit 055f7c7 to get more accurate results

@@           Coverage Diff           @@
##           master     #686   +/-   ##
=======================================
  Coverage   78.54%   78.54%           
=======================================
  Files          65       65           
  Lines        7141     7141           
  Branches     1565     1565           
=======================================
  Hits         5609     5609           
  Misses       1228     1228           
  Partials      304      304           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5937cb...055f7c7. Read the comment docs.

[bdice]

Comments attached. Overall I would be happy to see this document streamlined — I think less content is better than more in the template. Large blocks of instructions are often ignored, in my experience with creating PRs on other projects and seeing PRs from new contributors on projects I maintain. Maybe we can outsource more content to the CONTRIBUTING document.

[csadorf]

I agree with @bdice 's comments. Please address those and then re-request review from me. Thank you.

[csadorf]

I have one minor suggestion, but overall this is fine.

I still feel like the list is overall maybe a bit too long. Lack of testing is automatically controlled by codecov for example. Code style is automatically checked by linters and autoformatters. How would you feel about dropping all items that should be automatically enforced? @bdice Also interested in your opinion here.

[cbkerr]

Code style is automatically checked by linters and autoformatters.

I like getting rid of the code style check, since that is part of the code guidelines

Lack of testing is automatically controlled by codecov for example

I think having the box for tests would help remind me to include tests while I'm starting the PR as opposed to when reviews are nearly done.

[bdice]

I still feel like the list is overall maybe a bit too long. Lack of testing is automatically controlled by codecov for example.

Code coverage isn't really a metric of whether code is tested adequately. I wouldn't rely on codecov alone, since it is very prone to noise and doesn't really indicate whether the code has bugs -- only that it executes. Removing a line of documentation decreases net coverage, while splitting a single line of code into multiple lines sometimes increases coverage...

Code style is automatically checked by linters and autoformatters. How would you feel about dropping all items that should be automatically enforced?

Code style can be removed from the checklist, that is sufficiently enforced by our CI at this point.

[cbkerr]

I think this is ready for final review (even though not all is checked off). I updated the PR description with the new checklist.
Do I need to update the changelog?

[bdice]

Do I need to update the changelog?

Nope, this kind of thing isn't put in the changelog (that's also a funny question, in a "meta" way 😉).

[bdice]

I approve. This is (after many iterations and great patience by all involved) an improvement over the current PR template! Thanks @cbkerr. The final form of this PR will need to be copied to other signac projects. Note that some templates may be different, like signac-docs. Similarly, signac-dashboard doesn't currently track contributors in the same way as signac and signac-flow (I have used git shortlog -sne > contributors.txt at release time, for simplicity). Do whatever makes the most sense to you for those cases.

[bdice]

It looks like we have a test failure related to pytest-xdist. It looks like worker 0 collected some tests for MongoDB that were not collected by worker 1, and one of those tests failed. 😕 I guess we have a few paths forward:

• Disable pytest-xdist
• Only run signac tests in parallel and run synced collections tests separately
• Ignore this failure and hope it's infrequent

csadorf is on vacation and this has enough reviews.

[vyasr]

The two workers collecting different threads is a symptom, not the problem. If you look at the failure, it's in the code that actually decides whether or not to run MongoDB tests by testing whether a MongoDB server can be accessed. The problem is almost certainly coming from a race condition between the two processes in performing the test write to the same MongoDB server. The brute force solution is to replace the except clause with a blanket except Exception. Worst case we just don't run MongoDB tests. If we want to be a little safer to avoid failing in serial contexts, we could detect whether or not we're running with pytest-xdist before deciding whether to allow arbitrary exceptions through.