Infos for contributers on prs and reviews in separate pages

docs/source/developer-guides/index.rst

@@ -6,5 +6,6 @@ The following pages provide information for existing and prospective developers
 .. toctree::
    :maxdepth: 1
 
-   PRs and Reviews <prs-and-reviews.rst>
+   Creating Pull-Requests <prs.rst>
    Coding Standards <coding-standards>
+   Giving Reviews <reviews.rst>

docs/source/developer-guides/prs.rst

@@ -0,0 +1,61 @@
+PRs
+===
+
+The goal of the pull-request and review process is to ensure that the proposed change is useful and maintainable over the long run. Submitters should consider and reviewers should evaluate the below criteria.
+
+PR Description
+---------------
+
+- Have a meaningful title: it's easier when creating the changelog or when searching through old PRs
+- New features add code that needs to be maintained and tested, and risk introducing bugs. You need to motivate why the PR should be merged
+- Explain what the PR does in the description: it makes it easier/faster to review
+- Make the PR as small as possible: it's much easier to review five PRs 200 lines each than one single PR with 1000 lines
+- If a PR is known to create conflict with other active PRs, try to coordinate, and link to each other
+  - If appropriate, explain the desired review/merge order
+- For complex changes that will require multiple PRs, create an issue to keep track of what has been done and what's left to do.
+- Is the PR focused on a single bugfix or feature?
+  - Consider moving self contained changes to separate PRs
+
+Public Interfaces
+-----------------
+
+- Are there comprehensive tests
+- Does the interface meet the use case - and for that matter is there a use case description
+  - Is the usage intuitive
+  - Do we really want this as a public interface, and maintain it?
+- Is the interface API consistent with existing ones?
+  - E.g. if everything else takes execution space instances as the first argument, don't make it the last argument
+  - Does the naming style match other Kokkos APIs
+- Are the interface semantics consistent with existing ones?
+  - If everything else (or at least the majority) of interfaces taking an execution space instance argument is async, then new interfaces taking one should be too
+- For any routine that's going to launch any parallel work, does it have an overload that takes an execution space instance, and does that overload use the instance for all parallel work?
+- If the functionality is similar to an ISO C++ functionality, is the interface and behavior similar, and in places where it's not is it a conscious decision
+- Is there a corresponding API documentation PR
+- Are the corner cases handled (works correctly, won't compile, detected at run time)?
+- Do the C++ defaulted functions do the right thing (including if needed to be marked with KOKKOS_DEFAULTED_FUNCTION)?
+
+
+Internal Implementation
+-----------------------
+
+- Is the implementation style consistent with the rest of Kokkos
+- Is there unnecessary code duplication
+  - In particular: is code that can be shared across backends shared?
+- Did it go in the right sub-part of Kokkos (core, algorithms, containers etc.)
+- Are there debug checks we should add?
+  - Like do the arguments make sense together etc.
+- Are implementation details accidentally exposed?
+- Is there unnecessary fencing
+- Are there unnecessary allocations/deallocations
+- Avoiding tangle of inclusions: are headers/files including only what is needed?
+- Is the code expressing intent clearly (choosing expressive names for variables, functions, etc)?
+- Are changes in the code and intent properly captured in the description of the PR?
+- Consider appropriateness of tests for implementation details
+  - We want to avoid needing to touch many tests for changes in on internal details
+
+Tests
+---------------
+
+- For bug fix PRs: add test which would catch the issue without the fix
+- Do newly added tests have the correct granularity?
+- Do tests have a suitable runtime or are unnecessarily large?

docs/source/developer-guides/reviews.rst

@@ -0,0 +1,92 @@
+Reviews
+=======
+
+The goal of a review is to help the code contributor to improve the code while also checking if it is the best approach to the described problem.
+
+Reviewer Behavior
+-----------------
+
+- Provide timely feedback and respond to changes by the author of the pull request in a reasonable amount of time; it's best to give feedback to pull requests as quickly as possible.
+- Only request changes if they are ready to resolve the request upon changes by the author of the pull request; stalling pull requests for requested changes that have been addressed is a problem.
+- Only review pull requests that have been marked as ready; we have a bunch of pull requests that explore the feasibility of ideas and just need the CI to run. Similarly, pull requests should only be marked as "ready for review" if the author is reasonably happy with the status. If the author mostly seeks feedback on general design and direction, this should be clearly communicated in the pull request description (either "draft" or "ready for review").
+- Mirror communication with pull request author outside of pull requests (on slack, in person, video calls, etc.) in comments to the pull request.
+- Contact authors directly if more clarification is needed.
+- Don't be afraid of reviewing pull requests even if they are (slightly) outside your comfort zone.
+- Work with authors to bring issues/questions that need a quorum/discussion with a larger audience to the developer meeting.
+
+Checklist
+=========
+
+The following checklist can serve as guidance for a thorough review. It is extensive and tries to be general, thus it might be overkill for small PRs (e.g. simple bugfixes)
+
+Fundamental Questions
+---------------------
+
+- Is the PR title clear enough about the scope of the changes?
+- Is clear what problem the PR is trying to resolve?
+- Is the proposed solution appropriate for the problem?
+- Is it working (check the CI)?
+- Does the PR introduce/change abstractions? If yes, what behavior has changed?
+- Which other abstractions/classes/concepts in Kokkos interact with the change?
+- Is there a good reason for including this code?
+
+Design
+------
+
+- Does it adhere to design principles like SOLID,DRY?
+- Does the design and variable naming fit into the rest of Kokkos?
+- Is the current design restricting future design choices? Does/should it allow extension?
+- Is any implicit dpenendency introduced?
+- Is the design reasonably simple?
+- Are the used names descriptive of what something does?
+
+Complexity
+----------
+
+- How complex is the change? Should/can it be split into multiple PRs?
+- Is there an easier way to do it?
+- Should some corner cases be excluded?
+
+Interface and usecase
+---------------------
+
+- Is it aligned with other Kokkos interfaces?
+- Is the interface of all functions/classes/etc. intuitive? Does it have a descriptive name?
+- How is it used in code in the end?
+
+Documentation
+-------------
+
+- Does it need documentation? If yes, is it already in a PR?
+- Does it need comments in the implementation to express intent?
+
+Interactions
+------------
+
+- Does an abstraction/function have opaque dependencies on non-local variables?
+- Are interfaces or variables hidden or shadowed?
+- Are any non-local variables changed?
+
+Includes
+--------
+
+- Does the header use minimal includes?
+- Are the new headers self-contained?
+
+Run
+---
+
+- Does the CI work?
+- Can I run it and try to break it?
+
+Communication
+-------------
+
+- Am I using a helpful, neutral tone?
+- Am I giving enough info to the author?
+
+Final Questions
+---------------
+
+- Does it definitely improve the current state?
+- Am I willing to maintain this code/concept/abstraction/idea in the future?