development process updates

_toc.yml

@@ -8,9 +8,10 @@ parts:
     chapters:
       - file: contributing/setup
       - file: contributing/coding
-      - file: contributing/pr
+      - file: contributing/process-outline
+      - file: contributing/submitting-pr
       - file: contributing/troubleshooting
-      - file: contributing/developer-guide
+      - file: contributing/responding-pr-review
       - file: contributing/review
 
 

contributing/developer-guide.md → contributing/process-outline.md

@@ -1,23 +1,22 @@
-# Development process
+# Process outline
 
-This guide should give you an overview of what the process looks like, the different steps etc.
+This guide provides an overview of what the contribution process is like.
 
-The flow in general:
-1. Get an issue assigned.
+1. Get an issue assigned
 2. Describe what's the acceptance criteria (what I'm going to work on, what's the definition of done)
-3. Submit a PR.
+3. Submit a PR
 4. Review Process
 5. Demo
 
-Let's dive into each of these sections:
+Let's dive into each of these sections.
 
-## 1. Getting issue assigned: 
-- Start by identifying the issue that you're going to work 
-on from the project's issue tracker/or the one you got assigned.
-- The project's maintainers may assign you an issue, or 
-you can choose one that you're interested in working on.
+## 1. Getting issue assigned
+
+The project's maintainers may assign you an issue, or you can choose one that you're
+interested in working on.
+
+## 2. Describe the Acceptance Criteria
 
-## 2. Describe the Acceptance Criteria: 
 - Before starting to work on the issue, describe the acceptance criteria (AC). 
 - The AC stage should be resolve in **24 hours** from issue assignment.
 - Describing the criteria includes what you're going to work on,
@@ -34,26 +33,34 @@ you can choose one that you're interested in working on.
   5. Telemetry
   6. Release post/Demo (except for small doc changes/fixes). 
 - Once the scope is agreed, you can start working on the pull request. 
-- Each issue/PR should have a main reviewer, if you didn't see anyone aligned, 
-  please ask in the issue and tag us.
+- Unless otherwise noticed, the person who opened the issue should give the thumbs up for the acceptance criteria (the only exception is when someone outside of our team opened an issue in an open-source project, in such case, someone from the team should approve the acceptance criteria)
 - Once the criteria is set, update the pull request text to have that criteria as a check-list
   both you and the reviewer can reference.
 
-## 3. Submit a Pull Request:
-- Once you've completed the work, submit a pull request to the repository. 
-- The pull request will include the changes you've made and any relevant 
-  information or comments you want to share with your reviewer.
+## 3. Submit a Draft Pull Request
+
+```{note}
+For more information, see [](submitting-pr.md).
+```
+
+- If you want to receive early feedback, you can open a Draft Pull Request
+- Once you've completed the work, click on "Ready to Review" and request a review
 
-## 4. Review Process: 
-- Your pull request will be reviewed by the dedicated maintainer or other team members. 
-  They will provide feedback on your work, including any suggested changes or improvements. 
+## 4. Review Process
+
+```{note}
+For more information, see [](responding-pr-review.md).
+```
+
+- Your pull request will be reviewed by the maintainers. They will provide feedback on your work, including any suggested changes or improvements. 
 - Address their feedback and make any necessary changes to your work. 
-- This step shouldn't take more than **2-4 days**, and you should 
-focus on closing existing issues (your active PR) than taking new issues. 
-- Finally, once that's done, your changes will be merged into the main branch and the reviewer
+- This step shouldn't take more than **1-2 days**, and you should 
+focus on closing existing issues (your active PR) instead of taking new issues. 
+- Finally, once the PR is approved, your changes will be merged into the main branch and the reviewer
 will delete the branch.
 
-## 5. Demo: 
+## 5. Demo (Optional)
+
 - Once your pull request has been approved and merged, demonstrate your work to the team. 
   This helps to ensure that everyone is aware of the changes made and that they meet 
   the project's standards. It also helps us spread the word on what we're currently

contributing/responding-pr-review.md

@@ -0,0 +1,17 @@
+# Responding to PR reviews
+
+Once you get a review, these are the guidelines to follow.
+
+## Address all observations
+
+Address all review observations. That doesn't mean you should change your code on
+all observations but you should respond to all of them with a informative comment so
+the reviewer knows what you did (addressed the changes, have questions about their
+observations, etc.)
+
+## **Do not** mark the conversations as resolved
+
+The reviewer is responsible for making conversations as resolved.
+
+![](../assets/resolve-conversation.png)
+

contributing/review.md

@@ -1,7 +1,51 @@
 # Reviewing PRs
 
-## Guidelines
+## The first review should be thorough
 
-1. The first review should be throughout, so upcoming reviews become smaller
-2. Avoid adding new requirements to existing PRs, the code should be evaluated based on the acceptance criteria
-3. Share concise but direct feedback: strike a good balance between a short yet understandable message
+The first review should be the most detailed one, so upcoming reviews
+become smaller and take less time. The only exception is when there's something
+obviously broken (e.g., you ran some tests and the feature doesn't work as described in
+the acceptance criteria). In such case, you can comment what the problem is and tell
+the contributor to re-request your review once the problem is addressed.
+
+## Avoid adding new requirements to existing PRs
+
+The code should be evaluated based on the acceptance criteria and new items should
+not be added. However, if you test the PR and encounter that the implementation isn't
+complete, it's fine to bring it up. The most common scenario is when the new feature
+doesn't handle error correctly (e.g., unclear error messages), in such cases, you can
+ask the contributor to tackle it.
+
+## Share concise but direct feedback
+
+Strike a good balance between a short yet understandable message and detailed feedback.
+Written communication is hard, so minimize the number of words while maximizing the
+amount of information shared.
+
+Avoid using *it* or *this* in your comments, as it leads to ambiguity. Example:
+
+**Bad:** I noticed that it doesn't work if you pass `verbose=True`
+
+**Good**: I noticed that the function `perform_operation` doesn't work if you pass `verbose=True`
+
+## You're responsible for resolving conversations
+
+The reviewer is responsible for clicking on `Resolve conversation`. Contributors should address observations but it's up to the reviewer to mark them as resolved (as they're the ones that verify that their observations were effectively addressed).
+
+![](../assets/resolve-conversation.png)
+
+Once you've addressed all the comments, request another review.
+
+```{note}
+If you don't see the "Resolve conversation" button, tag `@edublancas` so he can fix GitHub permissions.
+```
+
+## It's ok to dismiss incomplete PRs
+
+If you receive a request for review but the PR is incomplete (e.g., the CI isn't passing) or there are obvious misses in the acceptance criteria, you can skip your review and check again the next day. It's the contributors responsibility to ensure their PRs are ready to review.
+
+## User-centric testing
+
+When reviewing new features, we encourage you to follow a user-centric approach and test the feature yourself (as opposed to just looking at the code and tests). Testing the new feature can help you uncover edge cases that the author didn't consider.
+
+For more information, see [](../maintainer/maintainer.md#quality-assurance).

contributing/setup.md

@@ -165,6 +165,48 @@ conda activate ENV_NAME
 You have to repeat this process for every project you contribute to. For example, if you start contributing to [JupySQL](https://github.com/ploomber/jupysql), and then you are contributing to [ploomber-engine](https://github.com/ploomber/ploomber-engine), you'll have to setup again.
 ```
 
+## Verify your installation
+
+If you follow these instructions for setting up, the package will install in editable
+mode. Meaning any changes to the source code will be visible when you start a session.
+
+To verify this you can run the following in your terminal:
+
+```sh
+python -c "import PACKAGE_NAME; print(PACKAGE_NAME)"
+```
+
+Change `PACKAGE_NAME` for the package you're contributing to. Usually the name of
+the repository is the name of the package, but there are exceptions, to ensure you
+got the right name, navigate to the repository and open the `src/` directory, the
+folders there are the package names. For example, in [JupySQL](https://github.com/ploomber/jupysql/tree/master/src), we have `sql/`, and
+`tests/`, so I can verify that JupySQL is installed in editable mode by running:
+
+```sh
+python -c "import sql; print(sql)"
+```
+
+If the package is in editable mode, you'll see something like this:
+
+```
+<module 'sql' from '/path/to/cloned/repository/src/sql/__init__.py'>
+```
+
+If not, you'll see something like this:
+
+```
+<module 'sql' from '/path/to/envs/tmp/lib/python3.X/site-packages/sql/__init__.py'>
+```
+
+If the package isn't correctly installed, you'll see:
+
+```
+ModuleNotFoundError: No module named 'sql'
+```
+
+If you encounter problems, message us on Slack.
+
+## Next
 
 Now, let's check your [fork and IDE settings.](coding)
 

contributing/pr.md → contributing/submitting-pr.md

@@ -90,6 +90,47 @@ Keep in mind this guidelines when writing changelog messages:
 
 ## Testing
 
+Testing our codes ensures that it's working as expected, so writing tests is an essential part of the contribution process. If you're contributing with a new feature, you'll have to write new tests from scratch. On the other hand, if you're fixing a bug, you might have to modify existing tests and add new tests to ensure that the bug has been fixed.
+
+Ensuring that your new feature works is just as important as ensuring that we're helping the user when things don't work as expected. For example, say you're working on a function named `add` that adds two numbers:
+
+```python
+def add(x, y):
+    return x + y
+```
+
+A test might look like this:
+
+```python
+def test_add():
+    assert add(21, 21) == 42
+```
+
+However, you should also take into account scenarios where the function doesn't work,
+for example, if the user calls the function with a string: `add("21", 21)`, they'll
+see the following error:
+
+```
+TypeError: can only concatenate str (not "int") to str
+```
+
+The error is difficult to understand, so you should ensure that you're covering likely
+scenarios (they don't have to be exhaustive but try to think what the user might do).
+After encountering such edge case, you can add a new test:
+
+
+```python
+def test_error_if_wrong_type():
+    with pytest.raises(TypeError) as excinfo:
+        add(21, 21)
+
+    message = "function add only supports numbers, you passed a str object in the first argument ('21')"
+    assert str(excinfo).value == message
+```
+
+Of course, the test won't pass since you haven't implemented such validation, but you can now go, implement it and verify that the test passes.
+
+
 * We use [pytest](https://docs.pytest.org/en/7.2.x/) for testing. A basic understanding of `pytest` is highly recommended to get started, especially [fixtures](https://docs.pytest.org/en/7.2.x/fixture.html), [parametrization](https://docs.pytest.org/en/7.2.x/parametrize.html), and [debugging](https://docs.pytest.org/en/7.2.x/how-to/failures.html)
 * In most cases, for a given in `src/ploomber/MODULE_NAME`, there is a testing module in `tests/MODULE_NAME`, if you're working on a particular module, you can execute the corresponding testing module for faster development but when submitting a pull request, all tests will run
 * If you're checking error messages and they include absolute paths to files, you may encounter some issues when running the Windows CI since the Github Actions VM has some symlinks. If the test calls `Pathlib.resolve()` ([resolves symlinks](https://docs.python.org/3/library/pathlib.html#id5)), call it in the test as well, if it doesn't, use `os.path.abspath()` (does not resolve symlinks).
@@ -182,7 +223,11 @@ To measure usage, we add telemetry to our packages. See the [user guide.](https:
 If the feature you're implementing requires extra packages, we might consider adding them as optional dependencies. [Check out the guide.](https://ploomber-core.readthedocs.io/en/latest/dependencies.html)
 
 
-## Opening a Pull Request
+## Opening a Draft Pull Request
+
+```{important}
+Assuming your PR passes all tests, **you should get a review within 24 hours.** If you don't, you can tag/notify the reviewer (they might've missed the notification).
+```
 
 To open a Pull Request, open the *original repository* on GitHub, then click on:
 
@@ -202,7 +247,58 @@ Then ensure the menu displays the original repository (`ploomber/PROJECT_NAME`)
 base repository: ploomber/PROJECT_NAME base: main (or master) ⬅️ head repository: YOURUSERNAME/PROJECT_NAME compare: YOURBRANCH
 ```
 
-Then click on open Pull Request.
+Then click on open **Draft Pull Request.**
+
+
+```{tip}
+If you open a **Ready to review PR** by mistake, you can convert it into a draft by
+clicking on "convert to draft" in the right side bar:
+
+![](../assets/convert-to-draft.png)
+```
+
+Opening a PR will trigger building documentation and running the tests. Wait for a few minutes (most of our tests run in
+<15 minutes). If the checks failed:
+
+![](../assets/checks-failed.png)
+
+Check the logs and fix them (see the [troubleshooting](troubleshooting.md) guide
+for tips). If you're unable to fix the issues after spending some time on them, you
+can message us on Slack.
+
+Once the tests pass:
+
+![](../assets/checks-passed.png)
+
+
+Ensure there are no [merge conflicts](#fixing-merge-conflicts).
+
+If all the test pass and there are no merge conflicts, you can [request a review](#requesting-a-review).
+
+```{important}
+If your PR isn't ready yet, open it as a draft. Keep it as a draft until it's ready for review.
+```
+
+## Requesting a review
+
+Once your PR is ready, you must request a review. In some of our repositories, the
+review is requested automatically once you click on "Ready for review":
+
+![](../assets/ready-for-review.png)
+
+But if the reviewers list looks empty, you can request a review by clicking on the
+gear icon ⚙️ in the reviewers section (in the right bar):
+
+![](../assets/reviewers.png)
+
+And then add `@edublancas` and `@idomic` as reviewers.
+
+![](../assets/request-review.png)
+
+```{note}
+If the gear ⚙️ button isn't visible, tag `@edublancas` in a comment so he can fix
+GitHub permissions.
+```
 
 ## Fixing merge conflicts
 

contributing/troubleshooting.md

@@ -1,14 +1,13 @@
-# Troubleshooting
-
+# Troubleshooting a PR
 
 If your PR is failing, use this troubleshooting guide.
 
 ## Unit tests failing
 
-First, ensure that the [linting passes.](pr.md#lintingformatting)
+First, ensure that the [linting passes.](submitting-pr.md#lintingformatting)
 
 
-If you see linting errors, fix them or run the [formatting command.](pr.md#lintingformatting)
+If you see linting errors, fix them or run the [formatting command.](submitting-pr.md#lintingformatting)
 
 
 ## Documentation
@@ -17,7 +16,7 @@ If your PR is failing due to documentation errors, [see here.](../documentation/
 
 ## Merge conflicts
 
-[See here.](pr.md#fixing-merge-conflicts)
+[See here.](submitting-pr.md#fixing-merge-conflicts)
 
 
 ## Other