Pull Requests

This document clarifies rules and expectations of contributing and reviewing pull requests. It is structured as a list of rules which can be referenced on a PR to moderate and drive discussions. If a rule causes distress during discussions itself, it has to be reviewed on the dev meeting and updated.

• Opening a Pull Request
• Requesting a Review
• Review Checklist
• Reviewing
• Landing
• Reverting
• Closing

Opening a Pull Request

<a name="pr-template"></a>

• 1. Each PR description has to follow the PR template

<a name="human-in-the-loop"></a>

• 2. Contributors can use whatever tools they would like to craft their contributions, but there must be a human in the loop. Contributors must read and review all LLM-generated code or text before they ask other project members to review it. The contributor is always the author and is fully accountable for their contributions. Contributors should be sufficiently confident that the contribution is high enough quality that asking for a review is a good use of scarce maintainer time, and they should be able to answer questions about their work themselves during review.

<a name="design-review"></a>

• 3. A PR can be opened early for the design review before going into the detailed implementation.
  • A request on the design review should be an explicit comment.
  • Such PR should be marked as a draft or with the WIP prefix.

<a name="fixups"></a>

• 4. Changes done after the PR has been opened should be kept in separate commits until the review process is finished. This allows reviewers to re-review only the updated parts of the PR and to determine what needs to be tested again. The "fixup" commits must be squashed before merging in order to keep a clean history.

Requesting a Review

<a name="review-reqs"></a>

• 1. A review can be requested when:
  • The PR template is filled in.
  • Changes are thoroughly tested by an author.
  • Changes thoroughly reviewed following the review checklist by an author. <a name="review-request-gh"></a>
• 2. A review can be requested explicitly using GitHub. <a name="review-request-comment"></a>
• 3. A review can be also requested as a comment from any GitHub users.
  • For example to invite the person who originally filed an issue for testing.

Review Checklist

<a name="checklist-build-and-test"></a>

• 1. The new code is built and tested according to the How to test section of a PR description. <a name="checklist-project-org"></a>
• 2. The new code is aligned with the project organization and coding conventions. <a name="checklist-breaking-changes"></a>
• 3. Breaking changes are justified and recorded in the changelog. <a name="checklist-dependencies"></a>
• 4. New dependencies are justified and verified.
  • For newly added dependencies, we run the license check workflow, but not in review mode.
    • If the license check reveals that a review is needed for the new dependency (i.e., ERROR: Found results that aren't part of the baseline! X some-dependency, some-license), we need to run the license check in review mode (npm run license:check:review).
    • Since we have no PAT secret defined for the repo at the moment, the license check in review mode needs to be done locally, either by the contributor (if they are a Theia committer) or by the reviewer. <a name="checklist-copied-code"></a>
• 5. Copied code is justified and approved via a CQ.
  • Look closely at the GitHub actions running for your PR: the 3pp/dash license check should be green.
  • If red: it most likely mean you need to create a CQ. <a name="checklist-copyright"></a>
• 6. Each new file has proper copyright with the current year and the name of contributing entity (individual or company). <a name="checklist-sign-off"></a>
• 7. Commits are signed-off: https://github.com/eclipse-theia/theia/blob/master/CONTRIBUTING.md#sign-your-work. <a name="checklist-meaningful-commits"></a>
• 8. Each commit has meaningful title and a body that explains what it does. One can take inspiration from the What it does section from the PR. <a name="checklist-commit-history"></a>
• 9. Commit history is rebased on master and contains only meaningful commits and changes (less are usually better).
  • For example, use git pull -r or git fetch && git rebase to pick up changes from the master. <a name="checklist-i18n"></a>
• 10. User-facing text is internationalized using the nls service.
  • For details, please see the Internationalization/Localization section in the Coding Guidelines.

Reviewing

<a name="reviewing-template"></a>

• 1. Reviewers should check that a PR has a proper description. <a name="reviewing-fn"></a>
• 2. Reviewers should build and verify changes according to the How to test section of a PR description. <a name="reviewing-checklist"></a>
• 3. Reviewers should ensure that all checks from the review checklist are successful. <a name="reviewing-share"></a>
• 4. A reviewer does not need to ensure everything but can verify a part of it and provide feedback as a comment. <a name="review-consultation"></a>
• 5. For any change that substantially alters the behavior of the application or one of its components, reviews should be requested from representatives of several contributing organizations to ensure consistency with the goals of the project and compatibility with significant adopters' downstream products.

Requesting Changes

<a name="changes-review-reqs"></a>

• 1. Changes should be requested if an author does not follow the review requirements. <a name="changes-no-nit"></a>
• 2. Changes cannot be requested because of the personal preferences of a reviewer.
  • Such change requests should be dismissed. <a name="changes-no-out-of-scope"></a>
• 3. Changes cannot be requested if they address issues out of the scope of a PR.
  • Such change requests should be dismissed and an issue should be filed to address them separately. <a name="changes-style-agreement"></a>
• 4. Styles and coding preferences should not be discussed on the PR, but raised in the dev meeting, agreed by the team, applied to the coding guidelines and after that followed by all contributors.

Approving

<a name="justifying-approve"></a>

• 1. Each approval should have supporting comments following these guidelines. <a name="dismissing-approve"></a>
• 2. An approval without a comment should be dismissed. <a name="approval-finality"></a>
• 3. Approval of a PR implies that the reviewer is prepared to merge the PR. A reviewer should only approve a pull request that they are prepared to merge it. If a PR is under review by multiple reviewers, reviewers who are not satisfied with the state of the PR should block its merge, for example by marking their review 'request changes'.

Collaborating

<a name="collaboration-on-pr"></a>

• 1. If a change request is important, but cannot be elaborated by a reviewer, then a reviewer should be encouraged to open an alternative PR or collaborate on a current PR. <a name="completing-pr"></a>
• 2. If a PR is important, but an author cannot or does not want to address outstanding issues, then maintainers can complete the PR with additional commits provided that the original author accepted the ECA and their commits are preserved - the original author's work should not be squashed away. <a name="suggesting-help-on-pr"></a>
• 3. Reviewers should offer their help via a comment to avoid intervening in an author's work. <a name="landing-stale-pr"></a>
• 4. Such comment is not required if an author is not responsive.

Landing

<a name="landing-pr"></a>

• 1. A PR can be landed when:
  • CI build has succeeded.
  • The author has accepted the Eclipse Contributor Agreement.
  • All checks from the review checklist are approved by at least one reviewer.
  • There are no unresolved review comments. <a name="merging-pr"></a>
• 2. Pull requests satisfying the criteria above should be merged in a timely fashion to avoid a buildup of approved PR's at release time. Responsibility for merging a PR falls to
  • The author, if the author is a committer.
  • A committer from the author's organization, if one is available.
  • The approving reviewer, otherwise.

Reverting

<a name="reverting-pr"></a>

• 1. If a PR causes regressions after landing then an author and maintainers have 2 days to resolve them after that a PR has to be reverted.

Closing

<a name="closing-pr"></a>

• 1. A reviewer cannot close a PR without a reason. <a name="closing-pr-reasons"></a>
• 2. A PR may be closed, for example, because of the following reasons:
  • It introduces functionality which should be implemented as external Theia or VS Code extensions.
  • It introduces structural or API changes between core extensions. Such changes have to be done by an experienced maintainer to avoid regressions and long reviews.
  • It should be a 3rd party component, e.g. Theia is not a logging framework or a proxy server.
  • It changes development infrastructure, e.g. testing frameworks, packaging and so on. Such changes have to be done by active maintainers after agreement in the dev meeting.
  • It violates the human-in-the-loop policy.