docs: Better documentation for developers

docs/development/code_review.md

@@ -0,0 +1,81 @@
+# Code review
+
+This document describes
+general guidelines for developers
+when reviewing a Pull Request (PR).
+
+## Verify that the PR solves the addressed issue
+
+### Understand the issue
+
+* Read the issue that was addressed by the PR and make sure you understand it
+* Read any discussions that occurred on the issue page
+
+### Check the Pull Request
+
+* Check the Pull Request for __completeness__: Was every point from the issue covered?
+* Check for __correctness__: Were the problems described in the issue solved in the intended way?
+* (For PRs that introduce new features:) Check the design - does it comply with our [guidelines][guidelines-general]?
+* Check for potential bugs - edge cases, exceptions that may be raised in functions that were called etc.
+* Check the code style: Is it readable? Do variables have sensible names? Is it consistent to existing code? Is there a better / more elegant way to do certain things (e.g. f-string instead of manual string concatenation)?
+* Check any warnings reported by your IDE
+
+## Check the tests
+
+* Run the tests locally
+* Check if there are any warnings that are not caught by the test suite
+* Make sure the test cases are complete - do they cover all edge cases (e.g. empty tables)?
+* Make sure they comply to our [guidelines][guidelines-tests] - are the tests parametrized and do all testcases have descriptive IDs?
+
+## Verify that the PR does not break existing code
+
+This is largely covered by the automated tests.
+However, you should always:
+
+* Make sure all tests actually ran through (pytest, linter, code coverage)
+* Make sure that the branch is up-to-date with the `main` branch, so you actually test the behaviour that will result once the feature branch is merged
+
+## Check the Pull Request format
+
+* Check that the PR title starts with a fitting [type](https://github.com/Safe-DS/.github/blob/main/.github/CONTRIBUTING.md#types) (e.g. `feat`, `fix`, ...)
+* Check that the changes introduced by the PR are documented in the PR message
+* Check that the `time spent by team` has been updated on the issue page
+
+## Requesting changes
+
+If you found any issues with the reviewed PR,
+navigate to the `Files changed` tab in the PR page
+and click on the respective line
+to add your comments.
+
+For more details on specific review features,
+check out the [documentation on GitHub][github-review].
+
+## Finishing the review
+
+When done, finish your review
+by navigating to the `Files changed` tab
+and clicking the green `Review changes` button
+on the top right.
+
+If you found no issues,
+select the `Approve` option
+to approve the changes made by the PR.
+If you like, you can add a "LGTM" meme
+from [lgtm.party](https://lgtm.party/),
+preferably one with a cat image.
+
+If no other reviewer suggested changes either,
+please also mark the corresponding issue
+as `Ready to merge` on the issue's page
+to update its status on the issue board.
+
+If you found any problems,
+select `Request Changes` instead.
+In this case, please also
+mark the corresponding issue
+as `In progress` again.
+
+[guidelines-general]: https://stdlib.safeds.com/en/stable/development/guidelines/
+[guidelines-tests]: https://stdlib.safeds.com/en/stable/development/guidelines/#tests
+[github-review]: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests

docs/development/guidelines.md

@@ -56,6 +56,43 @@ Some flag parameters drastically alter the semantics of a function. This can lea
     table.drop("name", axis="columns")
     ```
 
+### Return copies of objects
+
+Modifying objects in-place
+can lead to surprising behaviour
+and hard-to-find bugs.
+Methods shall never change
+the object they're called on
+or any of their parameters.
+
+!!! success "**DO** (library code):"
+
+    ```py
+        result = self._data.copy()
+        result.columns = self._schema.column_names
+        result[new_column.name] = new_column._data
+        return Table._from_pandas_dataframe(result)
+    ```
+
+!!! failure "**DON'T** (library code):"
+
+    ```py
+        self._data.add(new_column, axis='column')
+    ```
+
+The corresponding docstring should explicitly state
+that a method returns a copy:
+
+!!! success "**DO** (library code):"
+
+    ```py
+    """
+    Return a new table with the given column added as the last column.
+    The original table is not modified.
+    ...
+    """
+    ```
+
 ### Avoid uncommon abbreviations
 
 Write full words rather than abbreviations. The increased verbosity is offset by better readability, better functioning auto-completion, and a reduced need to consult the documentation when writing code. Common abbreviations like CSV or HTML are fine though, since they rarely require explanation.
@@ -205,27 +242,6 @@ The user should not have to deal with exceptions that are defined in the wrapper
         return pd.read_csv(path) # May raise a pd.ParserError
     ```
 
-### Sort entries in `__all__` lists alphabetically
-The entries in the `__all__` list in `__init__.py` files should be sorted alphabetically. This helps reduce the likelihood of merge conflicts when new entries are introduced on different branches.
-!!! success "**DO** (library code):"
-
-    ```py
-    __all__ = [
-        "ColumnSizeError",
-        "DuplicateColumnNameError",
-        "MissingValuesColumnError"
-    ]
-    ```
-!!! failure "**DON'T** (library code):"
-
-    ```py
-    __all__ = [
-        "MissingValuesColumnError",
-        "ColumnSizeError",
-        "DuplicateColumnNameError"
-    ]
-    ```
-
 ### Group API elements by task
 
 Packages should correspond to a specific task like classification or imputation. This eases discovery and makes it easy to switch between different solutions for the same task.
@@ -258,6 +274,49 @@ Passing values that are commonly used together around separately is tedious, ver
     training_feature_vectors, validation_feature_vectors, training_target_values, validation_target_values = split(feature_vectors, target_values)
     ```
 
+## Code style
+
+### Consistency
+
+If there is more than one way
+to solve a particular task,
+check how it has been solved
+at other places in the codebase
+and stick to that solution.
+
+### Sort exported classes in `__init__.py`
+
+Classes defined in a module
+that other classes shall be able to import
+must be defined
+in a list named `__all__`
+in the module's `__init__.py` file.
+This list should be sorted alphabetically,
+to reduce the likelihood of merge conflicts
+when adding new classes to it.
+
+!!! success "**DO** (library code):"
+
+    ```py
+    __all__ = [
+    "Column",
+    "Row",
+    "Table",
+    "TaggedTable",
+    ]
+    ```
+
+!!! failure "**DON'T** (library code):"
+
+    ```py
+    __all__ = [
+    "Table",
+    "TaggedTable",
+    "Column",
+    "Row",
+    ]
+    ```
+
 ## Docstrings
 
 The docstrings **should** use the [numpydoc](https://numpydoc.readthedocs.io/en/latest/format.html) format. The descriptions **should not** start with "this" and **should** use imperative mood. Refer to the subsections below for more details on how to document specific API elements.
@@ -370,7 +429,43 @@ Examples
 
 ## Tests
 
-If a function contains more code than just the getting or setting of a value, automated test should be added to the [`tests`][tests-folder] folder. The file structure in the tests folder should mirror the file structure of the [`src`][src-folder] folder.
+If a function contains more code
+than just the getting or setting of a value,
+automated test should be added
+to the [`tests`][tests-folder] folder.
+The file structure in the tests folder
+should mirror the file structure
+of the [`src`][src-folder] folder.
+
+Names of test functions
+shall start with `test_should_`
+followed by a description
+of the expected behaviour,
+e.g. `test_should_add_column`.
+
+### Testcases
+
+Tests should be parametrized
+using `@pytest.mark.parametrize`,
+even if there is only a single testcase.
+Testcases should be given
+descriptive IDs.
+
+Example:
+
+    ```py
+    @pytest.mark.parametrize(
+        ("path", "expected"),
+        [
+            ("table.csv", Table({"A": [1], "B": [2]})),
+            (Path("table.csv"), Table({"A": [1], "B": [2]})),
+            ("empty_table.csv", Table()),
+        ],
+        ids=["by string", "by path", "empty"],
+    )
+    def test_should_create_table_from_csv_file(path: str | Path, expected: Table) -> None:
+        ...
+    ```
 
 [src-folder]: https://github.com/Safe-DS/Stdlib/tree/main/src
 [tests-folder]: https://github.com/Safe-DS/Stdlib/tree/main/tests

mkdocs.yml

@@ -17,6 +17,7 @@ nav:
     - Environment: development/environment.md
     - Guidelines: development/guidelines.md
     - Contributing: https://github.com/Safe-DS/Stdlib/contribute
+    - Code Review Guide: development/code_review.md
 
 # Configuration of MkDocs & Material for MkDocs --------------------------------