diff --git a/_toc.yml b/_toc.yml index be7e237..2d24207 100644 --- a/_toc.yml +++ b/_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 diff --git a/assets/checks-failed.png b/assets/checks-failed.png new file mode 100644 index 0000000..0790bc5 Binary files /dev/null and b/assets/checks-failed.png differ diff --git a/assets/checks-passed.png b/assets/checks-passed.png new file mode 100644 index 0000000..32b80eb Binary files /dev/null and b/assets/checks-passed.png differ diff --git a/assets/convert-to-draft.png b/assets/convert-to-draft.png new file mode 100644 index 0000000..cef3c63 Binary files /dev/null and b/assets/convert-to-draft.png differ diff --git a/assets/ready-for-review.png b/assets/ready-for-review.png new file mode 100644 index 0000000..a613545 Binary files /dev/null and b/assets/ready-for-review.png differ diff --git a/assets/request-review.png b/assets/request-review.png new file mode 100644 index 0000000..0964c62 Binary files /dev/null and b/assets/request-review.png differ diff --git a/assets/resolve-conversation.png b/assets/resolve-conversation.png new file mode 100644 index 0000000..9be41d1 Binary files /dev/null and b/assets/resolve-conversation.png differ diff --git a/assets/reviewers.png b/assets/reviewers.png new file mode 100644 index 0000000..b989c81 Binary files /dev/null and b/assets/reviewers.png differ diff --git a/contributing/developer-guide.md b/contributing/process-outline.md similarity index 52% rename from contributing/developer-guide.md rename to contributing/process-outline.md index ce10fc5..4742fbf 100644 --- a/contributing/developer-guide.md +++ b/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 diff --git a/contributing/responding-pr-review.md b/contributing/responding-pr-review.md new file mode 100644 index 0000000..4be98ed --- /dev/null +++ b/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) + diff --git a/contributing/review.md b/contributing/review.md index 2b3b359..8b77944 100644 --- a/contributing/review.md +++ b/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 \ No newline at end of file +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). \ No newline at end of file diff --git a/contributing/setup.md b/contributing/setup.md index 48682e1..e1402cf 100644 --- a/contributing/setup.md +++ b/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: + +``` + +``` + +If not, you'll see something like this: + +``` + +``` + +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) diff --git a/contributing/pr.md b/contributing/submitting-pr.md similarity index 75% rename from contributing/pr.md rename to contributing/submitting-pr.md index dc13d1d..8d8b4d0 100644 --- a/contributing/pr.md +++ b/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 diff --git a/contributing/troubleshooting.md b/contributing/troubleshooting.md index c0788bf..02eec6f 100644 --- a/contributing/troubleshooting.md +++ b/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 diff --git a/maintainer/maintainer.md b/maintainer/maintainer.md index 74faebc..e20b63d 100644 --- a/maintainer/maintainer.md +++ b/maintainer/maintainer.md @@ -28,7 +28,7 @@ Currently, we're testing this with some members of the team (in alphabetical ord When performing a code review, verify the following: - Unit tests have been added and they're rigorously testing the code changes -- An appropriate [CHANGELOG](../contributing/pr.md#changelog) entry has been added (when needed) +- An appropriate [CHANGELOG](../contributing/submitting-pr.md#changelog) entry has been added (when needed) - The code meets the quality bar - Re-usable (e.g., abstracts common patterns in functions) - Clear (i.e., descriptive variable names, inline comments when needed) - this also applies to unit tests @@ -36,8 +36,8 @@ When performing a code review, verify the following: Owners also have the following responsibilities: -- If breaking API changes are introduced, a PR is merged with a [deprecation warning](../contributing/pr.md#backwards-compatibility) -- If breaking API changes are introduced: a [major version bump](../contributing/pr.md#backwards-compatibility) is performed +- If breaking API changes are introduced, a PR is merged with a [deprecation warning](../contributing/submitting-pr.md#backwards-compatibility) +- If breaking API changes are introduced: a [major version bump](../contributing/submitting-pr.md#backwards-compatibility) is performed - Ensure that all CI checks passed before merging a Pull Request - [Only applicable for PRs from external contributors] Approve CI executions (when an external contributor opens a PR, someone from the team needs to approve the CI run by clicking on a button) - If the CI fails, provide guidance to the contributor. If you suspect the CI is broken due an external factor (e.g., a dependency that its API), send a message on Slack @@ -46,6 +46,8 @@ Owners also have the following responsibilities: ## Quality assurance +### Testing with Binder + The easiest way to test code contributions is via Binder (a hosted JupyterLab). When reviewing a pull request, click on the [documentation link](../maintainer/doc-guide.md#previewing-docs). @@ -89,6 +91,20 @@ When testing the code: put yourself in the user's shoes (who has never executed > the latter case we should merge it and you can open a new issue to discuss further > improvements. +### Testing locally + +Alternatively, you can use the [GitHub CLI](https://cli.github.com/) to checkout PRs locally: + +```sh +cd path/to/project +gh checkout pr NUMBER +``` + +```{note} +Ensure that the package you're testing is installed in editable mode. +See [](../contributing/setup.md#verify-your-installation) for more information. +``` + ## Continuous integration We use GitHub Actions to test our projects. Each one should test against these configuration: