Doc: review guidelines

.github/pull_request_template.md

@@ -3,10 +3,14 @@ targeting and what it's changing.
 
 ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"
 
-## What is this fixing or adding?
+## What is this adding, removing or changing?
+
+
+## Why is the change being made?
 
 
 ## How was this tested?
+(Give information so that reviewers can reproduce your testing.)
 
 
 ## If this makes graphical changes, please attach screenshots.

docs/contributing.md

@@ -1,49 +1,85 @@
 # Contributing
 
+Development happens on GitHub, discussions may happen on [Discord](https://archipelago.gg/discord). Proposing code
+changes is done by opening a Pull Request on GitHub.
+
 All contributions are welcome, though we have a few requests of contributors, whether they be for core, webhost, or new 
 game contributions:
 
+## Worlds
+
+We consider worlds ("game implementations") as dependencies of Archipelago.
+
+* If you want to contribute to an existing world, please check if the world has contributing guidelines or get in touch
+  with the world maintainer. Maintainers for worlds that are in this repository are typically listed in the
+  [CODEOWNERS](./CODEOWNERS) file, but the best way to get in touch may be via the
+  [Discord](https://archipelago.gg/discord).
+
+* If you want to contribute a new world, or take on world maintainership, please consider the following:
+  * Dependencies need to be fit for purpose. This means e.g. reasonable performance, memory usage and load times as
+    well as stability (i.e. similar to other games, low failure rate in Multiplayer generations).
+    You can make decisions easier for Archipelago ("core") maintainers by employing similar contribution and review
+    guidelines as Archipelago has itself.
+  * The world maintainer is in charge of the world. Core maintainers should only have to check for security issues,
+    however this requires that all changes to a world get properly reviewed/tested by someone if it didn't happen
+    already before opening the Pull Request to update the world. If world maintainers open Pull Requests themselves,
+    it has to be made clear at what point it is ready to be merged, otherwise the world maintainer's approval means
+    it's ready.
+  * We strongly recommend making use of unit tests. Please take a look at the
+    [logic unit test documentation](/docs/tests.md).
+  * We have some expectations of world maintainers, see [world maintainer.md](./world%20maintainer.md).
+
+* For help with adding a new world, please take a look at the [adding games documentation](/docs/adding%20games.md),
+  which details what is required to implement support for a game, and has tips on to get started.
+
+## WebHost
+
+The Website has some extra requirements on top of core Archipelago.
+Please refer to the [WebHost README](/WebHostLib/README.md).
+
+## Core
+
 * **Follow styling guidelines.**
   Please take a look at the [code style documentation](/docs/style.md)
   to ensure ease of communication and uniformity.
 
-* **Ensure that critical changes are covered by tests.**
-  It is strongly recommended that unit tests are used to avoid regression and to ensure everything is still working.
-  If you wish to contribute by adding a new game, please take a look at
-  the [logic unit test documentation](/docs/tests.md).
-  If you wish to contribute to the website, please take a look at [these tests](/test/webhost).
+* **Ensure that non-trivial changes are covered by tests.**
+  If this is not possible, leave a note in the Pull Request explaining how to test the change.
+  For trivial changes, leave a note that the change is trivial.
+  If you wish to contribute to the website, take a look at [these tests](/test/webhost).
+  While primarily focused on world tests, check [logic unit test documentation](/docs/tests.md)
+  to see how to run tests.
 
 * **Do not introduce unit test failures/regressions.**
   Archipelago supports multiple versions of Python. You may need to download older Python versions to fully test
-  your changes. Currently, the oldest supported version
-  is [Python 3.11](https://www.python.org/downloads/release/python-31113/).
-  It is recommended that automated github actions are turned on in your fork to have github run unit tests after
+  your changes. See [running from source.md](./running%20from%20source.md)
+  for the oldest and newest version currently supported.
+  It is recommended that automated GitHub actions are turned on in your fork to have GitHub run unit tests after
   pushing.
   You can turn them on here:  
   ![Github actions example](./img/github-actions-example.png)
 
-* **When reviewing PRs, please leave a message about what was done.**
-  We don't have full test coverage, so manual testing can help.
-  For code changes that could affect multiple worlds or that could have changes in unexpected code paths, manual testing
-  or checking if all code paths are covered by automated tests is desired. The original author may not have been able
-  to test all possibly affected worlds, or didn't know it would affect another world. In such cases, it is helpful to
-  state which games or settings were rolled, if any.
-  Please also tell us if you looked at code, just did functional testing, did both, or did neither.
-  If testing the PR depends on other PRs, please state what you merged into what for testing.
-  We cannot determine what "LGTM" means without additional context, so that should not be the norm.
+* **Use draft Pull Requests (only) for discussion.**
+  Drafts can be used to ask questions about specific code snippets or general design. When opening one, make sure to
+  ask your question in the PR. Draft PRs should not receive a full review.
+  If the change is not done, and you don't have a question (yet), opening the PR early could hurt visibility later.
+  If you want your ongoing work to be discoverable on GitHub, you can open a tracking issue for it.
 
-Other than these requests, we tend to judge code on a case-by-case basis.
+* **Self-review your code changes**
+  according to our [review guidelines](./review%20guidelines.md)
+  including static analysis.
 
-For contribution to the website, please refer to the [WebHost README](/WebHostLib/README.md).
+* **Follow up on Pull Request comments.**
+  if a change was requested, push an update soon or leave a comment.
+  Nitpicks that are ignored by the author should still get a reply, so we know when the changes are complete.
 
-If you want to contribute to the core, you will be subject to stricter review on your pull requests. It is recommended
-that you get in touch with other core maintainers via the [Discord](https://archipelago.gg/discord).
+* **Help out by reviewing Pull Requests.**
+  Everyone can collaborate in testing and reviewing suggested changes, moving the project along faster.
+  Please follow the [review guidelines](./review%20guidelines.md) to do a full review, and leave a note that you did,
+  or let us know what parts you reviewed or tested in case it was not a full review.
+  We cannot determine what "LGTM" means without additional context, so that should not be the norm.
 
-If you want to add Archipelago support for a new game, please take a look at
-the [adding games documentation](/docs/adding%20games.md)
-which details what is required to implement support for a game, and has tips on to get started.
-If you want to merge a new game into the main Archipelago repo, please make sure to read the responsibilities as a
-[world maintainer](/docs/world%20maintainer.md).
+Other than these requests, we tend to judge code on a case-by-case basis.
 
 For other questions, feel free to explore the [main documentation folder](/docs), and ask us questions in the
-#ap-world-dev channel of the [Discord](https://archipelago.gg/discord).
+#ap-core-dev and #ap-world-dev channels of the [Discord](https://archipelago.gg/discord).

docs/review guidelines.md

@@ -0,0 +1,137 @@
+# Review Guidelines
+
+[Quick link to the checklist](#review-checklist).
+
+These guidelines apply to suggested changes to core Archipelago and WebHost. That is everything besides /worlds/ but
+including /worlds/generic/. Worlds may define their own guidelines in the respective source directory.
+
+A full review should go through the checklist. The author of a Pull Request should self-review either before or right
+after opening a Pull Request.
+
+## Review Checklist
+
+1.  If it's a draft only look at the sections the author asked about.
+    [Details](#1-draft-prs)
+2.  Does the contribution change program structure or strongly impacts maintainability?
+    If so, was this brought up and approved on Discord or another earlier discussion?
+    [Details](#2-structure--maintainability--risks)
+3.  Does the PR have tests, explain how to test or is the change trivial? Are the tests complete?
+    [Details](#3-testing)
+4.  Does the PR meet requirements without introducing unnecessary complication?
+    [Details](#4-unnecessary-complication)
+5.  Is the PR too big?
+    [Details](#5-pr-size)
+6.  Is the change self-documenting or adds documentation? Does it have surprising (side) effects?
+    [Details](#6-documentation--naming)
+7.  Looking at the surrounding code, are there missed refactoring opportunities?
+    [Details](#7-surrounding-code)
+8.  Look at static analyzer results (i.e. linters and type errors/warnings unless required by CI).
+    [Details](#8-static-analyzers)
+9.  Does the code change follow the style guide?
+    [Details](#9-style-guide)
+10. Does the change introduce bad practices, insecure functions or unnecessary (arithmetic) complexity?
+    Are there cases where the code may not finish running / is stuck in an endless loop?
+    [Details](#10-code--implementation-quality)
+11. Clarify which change requests / PR comments are nitpicks and which are critical.
+    [Details](#11-change-requests-and-nitpicks)
+
+## Examples and Details
+
+### 1. Draft PRs
+
+If there are no question in a draft PR, or they are unclear, point to *draft Pull Requests* section in
+[contributing.md](contributing.md).
+
+### 2. Structure / Maintainability / Risks
+
+Things like new dependencies, shuffling big portions of code around, adding big portions of code, adding new entry
+points, adding new endpoints or extending "bad" legacy code may introduce new bugs and/or impact future maintainability
+of the code.
+
+Changing those after opening the PR may invalidate all existing reviews, and should be considered before a full review.
+
+If such changes were not discussed beforehand, consider discussion in the PR or pointing them to Discord before looking
+at details.
+
+Ideally if this was pre-approved, there is a note like "as discussed on Discord".
+
+### 3. Testing
+
+Ideally all non-trivial code changes, including corner cases, are covered by unit tests.
+If tests are incomplete, consider asking the author to add tests.
+If the PR mentions how to test the change (manually), check if that appears to be complete.
+If it's not a trivial change, nor does it add (complete) tests, nor does it explain how to test, request a change and
+don't continue with the review.
+
+For code changes that could affect multiple worlds or that could have changes in unexpected code paths, manual testing
+or checking if all code paths are covered by automated tests is desired. The original author may not have been able to
+test all possibly affected worlds, or didn't know it would affect another world. In such cases, it is helpful to state
+which games or settings were rolled, if any.
+If testing the PR depends on other PRs, please state what you merged into what for testing.
+
+### 4. Unnecessary Complication
+
+Code that is too complicated where there is a simpler solution should be pointed out as such.
+
+It may hurt both the review and future maintenance of the code.
+
+### 5. PR Size
+
+In most cases, if a PR fixes a bug, it should not add features or change existing behavior that is unrelated.
+
+If a PR is big already, consider if parts should be split out into a separate PR. It's harder to review big PRs.
+
+### 6. Documentation / Naming
+
+Do function and variable names match what the function is actually doing?
+Relying on comments is not great because they can be outdated.
+
+For functions that are hard to describe in few words, consider if a doc string should be required.
+
+### 7. Surrounding Code
+
+If existing code can be adapted without lumping together functionality or if a common part can be extracted, this
+should be done. Depending on the size of the change and number of duplicated lines this may be a nitpick or a
+requirement for maintainability reasons.
+
+### 8. Static Analyzers
+
+Static analysis helps to find problems before they happen at runtime, such as mixing types or using unsafe features.
+
+CI generates reports in "Analyze modified files" for Python code that can be used.
+We recommend using mypy, pyright or basedpyright locally to check for type errors and
+ruff or flake8 for general warnings.
+See / use ruff.toml for agreed-on rules.
+
+We don't currently have a defined setup for any other languages.
+
+All new code should make use of type hints if possible.
+Only critical problems should block a PR, but it should be pointed out as a nitpick if there are any typing or linter
+problems in the new code.
+
+Additionally, individual files can be made to require passing strict type checking by adding them to
+`.github /pyright-config.json`.
+
+*In the future, an ignore-list and list of extensions for flake8 may be specified.*
+
+### 9. Style Guide
+
+The [Style Guide](style.md) is not just formatting, but may list *Dos and Don'ts* for each individual language.
+
+If there are only formatting problems, this may be a nitpick. If the change adds a lot of *Don't*, a change should be
+requested.
+
+### 10. Code / Implementation Quality
+
+Generate, Multiserver and WebHost are performance critical and are susceptible to DoS, so *code that works* may not
+be good enough.
+
+Code that may never finish (in edge cases) may not be acceptable.
+
+### 11. Change Requests and Nitpicks
+
+Use the "request changes" review feature if the changes are critical,
+if it's only nitpicks use the "comment" review feature.
+
+Try to use proper phrasing to signal if a change is deemed necessary or if it's up to the author.
+The author should respond in either case, but any back and forth may delay the change getting merged.