diff --git a/docs/operations/git/development_workflow.md b/docs/operations/git/development_workflow.md new file mode 100644 index 0000000..78fc0d3 --- /dev/null +++ b/docs/operations/git/development_workflow.md @@ -0,0 +1,136 @@ +# Standard Development Workflow +This document describes how to contribute to repositories under the Neon AI +GitHub organization. These best practices help to ensure contributions can be +efficiently reviewed and accepted by project maintainers while maintaining a +clear commit history and high standard of quality. + +## Branch Naming +When creating a new branch to work on changes, it is recommended to use the +format `_`, where is `feat`, `fix`, `doc`, or a +similar descriptor of the nature of changes being contributed and +is a brief description in kebab-case. For example, the branch that adds this +page to documentation is labeled `doc_developer-workflow`. +> This is similar to [Conventional Branch](https://conventionalbranch.org/), + though there are cases where `/` in a branch name can cause issues with + git ref parsing that naively assumes it can segment the complete ref on `/`. + +This naming convention is not a hard requirement for submitting changes, but +it is recommended to help quickly organize and manage branches. + +## Committing Changes +Since PRs will squash the commit history into a single commit on the base branch, +the feature branch commit history is unimportant. Developers are encouraged to +commit changes in whatever manner is most efficient for their workflow. +> [git-scm](https://git-scm.com/docs/git-commit) has a good description of how + the `git commit` command works and its application. + +## Pull Requests +PRs should always be opened with the repository default branch as the base +(`dev` in most cases). PR titles should be a short description of changes +in plain English; for clarity, they should not contain any reference to other +PR or Issue numbers. +> More information about Pull Requests can be found in the + [GitHub Docs](https://docs.github.com/en/pull-requests). + +### PR Description +PR descriptions should use the configured template if present so that changelogs +and release notes are consistent. Following the template, the Description section should +include a detailed description of changes; the organization is not specified, so +this may be a list of changes or a free-form description of the changes. The +description may include rationale for the changes and design decisions, though +this is not strictly required; the description should remain focused on the +changes contained in the PR. + +The Issues section should contain a bulleted list of related issues and PRs, +both in the current repository and other repositories. Any issues that are +closed by the PR should use the "Closes" keyword so that they are automatically +closed when the PR is merged. There may be related issues that are not closed, +but they should still be included here for reference; this helps when reviewing +issues since related changes will be clearly visible in the issue history. +> A bulleted list is specified here because GitHub will render more detailed + links in a bulleted list than it does in a regular text block. + +The Other Notes section is available to include any additional information or +context that is relevant to the PR. This is a good place to include links to +references, justification of design decisions, edge cases that are handled, or +any other background information. + +This format is not a hard requirement, but it is recommended to help PR +reviewers quickly understand the changes and to maintain a consistent format +when reviewing previous changes/releases. + +### PR Reviews +All PRs must receive an approving review before they may be merged with very +few exceptions. The review process is the most important control that ensures +we maintain a high standard of quality and that every change properly implements +its intent. The process for completing a review is described in [Reviewing a Pull Request](./review_pr.md). + +When a PR is complete, it should not be marked as a draft and one or more +reviewers should be requested. If the PR is not complete but a review is still +desired for feedback on WIP changes, or to discuss design decisions, it is best +to leave a comment with those specific questions, tagging the reviewer. + +### GitHub Actions +Many repositories have configured automation that runs when a PR is opened or +commits are added to an open PR. Some of these will not run until the PR is +marked ready for review, while others will run even on draft PRs. In either +case, a PR may not be merged until all checks have passed with no exceptions. + +### Merging PRs +A PR is ready to be merged when at least one approving review has been left and +all checks have passed. If there are any reviews left requesting changes, those +changes must be addressed and that reviewer must leave an approving review. For +example, if there are two reviews requesting changes, then both reviewers must +approve the PR before it may be merged. + +Once the PR is ready to be merged, it should be merged using the +"Squash and Merge" option. This ensures that the commit history of the base +branch remains clean and that the changes in the PR are represented as a +single commit with the PT title and description in the commit history. +> A PR should never be merged with a "Merge Commit" or "Rebase and Merge" as + this will create a complex commit history on the base branch. With more + commits, it becomes difficult to parse the commit history and rebasing other + feature branches is much more difficult. + +## Rebasing Branches +Since multiple PRs may be open at the same time, and merge order is generally +unknown, it is likely that a PR will need to be rebased on the base branch at +some point. If a single author has multiple open PRs, they may choose to +define the merge order by marking PRs as Drafts, or leaving comments in open +PRs to indicate an intended merge order. +> The [git-scm Cheat Sheet](https://git-scm.com/cheat-sheet#combine-diverged-branches) + has a good visual representation of what rebasing does. + +### Resolving Conflicts +When rebasing a branch, it is possible that merge conflicts will arise due to +changes in the base branch overlapping changes in the feature branch. When this +happens, the developer performing the rebase must make decisions about how to +resolve the conflicts. + +Since resolving conflicts is a potentially destructive operation, it is +recommended to create a backup of the feature branch being rebased; this may +be done locally or by creating a new branch on GitHub, based on the +feature branch. With a safe backup in place, resolve the conflicts and complete +the rebase. +> Note that because rebasing is basically re-playing each commit on the feature + branch, there may be changes that need to be resolved where the result does + not match either the base branch or the feature branch. + +Once the rebase is complete, the local feature branch should be force pushed to +the remote (overwriting the previous branch that lacked recent commits on the +base branch). In nearly all cases, the PR diff should look identical to what it +was before the rebase. + +### Rebase Tips +- If rebasing a complex feature branch, it may be easier to squash commits on + the feature branch before rebasing. This reduces the number of commits that + need to be rebased and avoids tricky merge conflicts that come up when the + feature branch has multiple changes to the same chunk of code. +- If during a rebase you find something has gone wrong, you can always abort + and try again using `git rebase --abort`. This will return the local branch + to the state it was before the rebase started. +- If you find the PR diff has changed in a way you didn't expect after force + pushing changes, you can look at the backup branch to compare the changes. + To resolve, you can either start over by using `git reset --hard ` + to return to the pre-rebase state, or you can create an additional commit to + manually correct the rebase errors. diff --git a/docs/operations/git/review_pr.md b/docs/operations/git/review_pr.md new file mode 100644 index 0000000..0cf5f46 --- /dev/null +++ b/docs/operations/git/review_pr.md @@ -0,0 +1,104 @@ +# Reviewing a Pull Request +This document describes the process for reviewing a pull request (PR). Specific +review criteria may apply to different projects, but this document aims to +cover the general process that applies to Neon AI repositories. +> See also [Project QA Standards](../qa_standards.md) for + more information about expectations for testing and + automation. + +## When to Review +A PR should generally be reviewed by requested reviewers when it is marked +"Ready for Review". If a review is requested on a Draft PR, the reviewer should +read the PR description and comments to understand if the author is looking for +specific feedback or input. +> Note that a PR may be marked "Ready for Review", but pending or failing + checks may result in further changes before the PR is actually ready. In + these cases, it is best to notify the PR author of the issues and hold a + full review until they are resolved. + +## Review Process +The purpose of PR reviews is to ensure that changes are complete, described +accurately and completely in the PR description, adequately tested, and that +any relevant issues are linked and resolved or otherwise partially addressed. + +### 1. Read the PR Title, Description, and Comments +The PR title should be a succinct and understandable statement of the changes +in the PR. If unclear, edit or ask the author to do so. The PR description +should provide an understandable summary of the changes and highlight any +specific trade-offs or design decisions. If any of those design decisions don't +make sense or there is a better alternative, quote the relevant line in a +comment to discuss with the PR author. Be sure to read the comment history as +well in case the decision has already been discussed. + +Once there is clear alignment on the goal of the PR and design discussions are +resolved, the code is ready for review. + +### 2. Review the Code Changes +Code changes should be reviewed for correctness, readability, consistency, and +scope. Much of the review process is subjective, so this document will only aim +to provide guidance for things to look for, rather than a rubric for evaluation. + +#### Correctness +Changes should obviously be free of syntax errors and other bugs. Ensure that +any changes come with test coverage or that they can be manually validated if +adding tests is not feasible. Consider any edge cases or potential exceptions +and confirm they are handled or at least acknowledged. If there are undefined +behaviors that are not explicitly handled or discussed in the PR, flag them as +issues to be resolved. + +#### Readability +Review new methods, functions, and variables for clear names and type annotations +as appropriate. New methods and functions should have docstrings that match the +format of other docstrings in the repository. If there is complex logic that +isn't clear, make sure it has a comment explaining its purpose. Ensure that +any exception handling includes clear logs and that any raised exceptions use +an appropriate exception type and message. + +#### Consistency +Ensure that changes are consistent with the existing codebase. New functions and +classes should integrate with existing modules where it makes sense and +signatures should follow the same format as existing code. Suggest changes as +appropriate to help the author address any issues. + +#### Scope +All changes in the PR should be constrained in scope to one specific feature, +fix, documentation task, or other discrete change. If a PR includes multiple +unrelated changes, flag the changes that are out of scope to request they be +separated into their own PR(s). This can be particularly helpful if some +changes are complete and ready to merge, while others have extensive feedback +to address. +> Note that this may lead to PRs that are dependent on each other. Use your + judgement to determine if this is appropriate, or if separating changes + will create more work for limited benefit. + +### 3. Complete the Review +Once changes have been reviewed and comments left, a review must be completed. +A review can be completed to Approve, Request Changes, or Comment on the PR. + +#### Approving Review +If there are no changes to be made and the PR is ready to be merged, leave an +approving review. This may be done with or without comment, but it is best +practice to leave a comment indicating what you did (i.e. tested changes, +reviewed code, reviewed tests, etc.) in case something comes up in the future +and the merged PR needs to be referenced. +> Note that an approving review may include comments that suggest a future + enhancement or consideration to be moved to a new Issue. These should be + addressed and resolved before merge, even though the PR is already approved. + +#### Requesting Changes +If there are issues noted in the review that must be addressed before the PR +is ready to merge, leave a review requesting changes. It is helpful to highlight +in the review comment what comments in particular need to be addressed and if +any of the comments are optional suggestions or open for discussion. + +#### Comment Only +If there is feedback that is not a specific request for changes and the PR is +not yet ready to be approved, a Comment review may be left. This will most +often be done when a PR is still a Draft and comments don't necessarily relate +to a specific change. This kind of review can also be helpful when changes are +reviewed, but further testing will be done before approval. +> If the PR author is reviewing changes, they are only allowed to leave a + Comment, since it would not make sense to approve or request changes on their + own PR. This can be a useful practice for reviewing changes before requesting + reviews from others. + diff --git a/mkdocs.yml b/mkdocs.yml index fae42a3..a5a2db1 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -138,6 +138,8 @@ nav: - Git: - Undo a Stable PR Workflow: operations/git/undo_stable_pr.md - Triage New GitHub Issues: operations/git/triage_issues.md + - Development Workflow: operations/git/development_workflow.md + - Pull Request Review: operations/git/review_pr.md - Skills: - Stable Release Workflow: operations/skills/workflow_stable_release.md - Alpha Release Workflow: operations/skills/workflow_alpha_release.md