Skip to content

Commit 719530e

Browse files
NeonDanielNeonDaniel
authored
Document development and PR review workflows (#77)
* Document expected development and PR review workflows * Update to address review feedback
1 parent 4372dd8 commit 719530e

3 files changed

Lines changed: 248 additions & 0 deletions

File tree

‎docs/operations/git/development_workflow.md‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
# Standard Development Workflow
2+
This document describes how to contribute to repositories under the Neon AI
3+
GitHub organization. These best practices help to ensure contributions can be
4+
efficiently reviewed and accepted by project maintainers while maintaining a
5+
clear commit history and high standard of quality.
6+
7+
## Branch Naming
8+
When creating a new branch to work on changes, it is recommended to use the
9+
format `<type>_<description>`, where <type> is `feat`, `fix`, `doc`, or a
10+
similar descriptor of the nature of changes being contributed and <description>
11+
is a brief description in kebab-case. For example, the branch that adds this
12+
page to documentation is labeled `doc_developer-workflow`.
13+
> This is similar to [Conventional Branch](https://conventionalbranch.org/),
14+
though there are cases where `/` in a branch name can cause issues with
15+
git ref parsing that naively assumes it can segment the complete ref on `/`.
16+
17+
This naming convention is not a hard requirement for submitting changes, but
18+
it is recommended to help quickly organize and manage branches. Note that
19+
branch names are case-sensitive, but it is recommended to use lowercase strings
20+
for consistency and to avoid potential conflicts on file systems that are not
21+
case-sensitive (i.e. APFS on macOS).
22+
23+
## Committing Changes
24+
Since PRs will squash the commit history into a single commit on the base branch,
25+
the feature branch commit history is unimportant. Developers are encouraged to
26+
commit changes in whatever manner is most efficient for their workflow.
27+
> [git-scm](https://git-scm.com/docs/git-commit) has a good description of how
28+
the `git commit` command works and its application.
29+
30+
## Pull Requests
31+
PRs should always be opened with the repository default branch as the base
32+
(`dev` in most cases). PR titles should be a short description of changes
33+
in plain English; for clarity, they should not contain any reference to other
34+
PR or Issue numbers.
35+
> More information about Pull Requests can be found in the
36+
[GitHub Docs](https://docs.github.com/en/pull-requests).
37+
38+
### PR Description
39+
PR descriptions should use the configured template if present so that changelogs
40+
and release notes are consistent. Following the template, the Description section should
41+
include a detailed description of changes; the organization is not specified, so
42+
this may be a list of changes or a free-form description of the changes. The
43+
description may include rationale for the changes and design decisions, though
44+
this is not strictly required; the description should remain focused on the
45+
changes contained in the PR.
46+
47+
The Issues section should contain a bulleted list of related issues and PRs,
48+
both in the current repository and other repositories. Any issues that are
49+
closed by the PR should use the "Closes" keyword so that they are automatically
50+
closed when the PR is merged. There may be related issues that are not closed,
51+
but they should still be included here for reference; this helps when reviewing
52+
issues since related changes will be clearly visible in the issue history.
53+
> A bulleted list is specified here because GitHub will render more detailed
54+
links in a bulleted list than it does in a regular text block.
55+
56+
The Other Notes section is available to include any additional information or
57+
context that is relevant to the PR. This is a good place to include links to
58+
references, justification of design decisions, edge cases that are handled, or
59+
any other background information.
60+
61+
This format is not a hard requirement, but it is recommended to help PR
62+
reviewers quickly understand the changes and to maintain a consistent format
63+
when reviewing previous changes/releases.
64+
65+
### PR Reviews
66+
All PRs must receive an approving review before they may be merged with very
67+
few exceptions. The review process is the most important control that ensures
68+
we maintain a high standard of quality and that every change properly implements
69+
its intent. The process for completing a review is described in [Reviewing a Pull Request](./review_pr.md).
70+
71+
When a PR is complete, it should not be marked as a draft and one or more
72+
reviewers should be requested. If the PR is not complete but a review is still
73+
desired for feedback on WIP changes, or to discuss design decisions, it is best
74+
to leave a comment with those specific questions, tagging the reviewer.
75+
76+
### GitHub Actions
77+
Many repositories have configured automation that runs when a PR is opened or
78+
commits are added to an open PR. Some of these will not run until the PR is
79+
marked ready for review, while others will run even on draft PRs. In either
80+
case, a PR may not be merged until all checks have passed with no exceptions.
81+
82+
### Merging PRs
83+
A PR is ready to be merged when at least one approving review has been left and
84+
all checks have passed. If there are any reviews left requesting changes, those
85+
changes must be addressed and that reviewer must leave an approving review. For
86+
example, if there are two reviews requesting changes, then both reviewers must
87+
approve the PR before it may be merged.
88+
89+
Once the PR is ready to be merged, it should be merged using the
90+
"Squash and Merge" option. This ensures that the commit history of the base
91+
branch remains clean and that the changes in the PR are represented as a
92+
single commit with the PT title and description in the commit history.
93+
> A PR should never be merged with a "Merge Commit" or "Rebase and Merge" as
94+
this will create a complex commit history on the base branch. With more
95+
commits, it becomes difficult to parse the commit history and rebasing other
96+
feature branches is much more difficult.
97+
98+
## Rebasing Branches
99+
Since multiple PRs may be open at the same time, and merge order is generally
100+
unknown, it is likely that a PR will need to be rebased on the base branch at
101+
some point. If a single author has multiple open PRs, they may choose to
102+
define the merge order by marking PRs as Drafts, or leaving comments in open
103+
PRs to indicate an intended merge order.
104+
> The [git-scm Cheat Sheet](https://git-scm.com/cheat-sheet#combine-diverged-branches)
105+
has a good visual representation of what rebasing does.
106+
107+
### Resolving Conflicts
108+
When rebasing a branch, it is possible that merge conflicts will arise due to
109+
changes in the base branch overlapping changes in the feature branch. When this
110+
happens, the developer performing the rebase must make decisions about how to
111+
resolve the conflicts.
112+
> Since these conflicts can be complicated to resolve, it may be best to defer
113+
to or collaborate with the developer(s) who are responsible for the
114+
conflicting changes.
115+
116+
Since resolving conflicts is a potentially destructive operation, it is
117+
recommended to create a backup of the feature branch being rebased; this may
118+
be done locally or by creating a new branch on GitHub, based on the
119+
feature branch. With a safe backup in place, resolve the conflicts and complete
120+
the rebase.
121+
> Note that because rebasing is basically re-playing each commit on the feature
122+
branch, there may be changes that need to be resolved where the result does
123+
not match either the base branch or the feature branch.
124+
125+
Once the rebase is complete, the local feature branch should be force pushed to
126+
the remote (overwriting the previous branch that lacked recent commits on the
127+
base branch). In nearly all cases, the PR diff should look identical to what it
128+
was before the rebase.
129+
130+
### Rebase Tips
131+
- If rebasing a complex feature branch, it may be easier to squash commits on
132+
the feature branch before rebasing. This reduces the number of commits that
133+
need to be rebased and avoids tricky merge conflicts that come up when the
134+
feature branch has multiple changes to the same chunk of code.
135+
- If during a rebase you find something has gone wrong, you can always abort
136+
and try again using `git rebase --abort`. This will return the local branch
137+
to the state it was before the rebase started.
138+
- If you find the PR diff has changed in a way you didn't expect after force
139+
pushing changes, you can look at the backup branch to compare the changes.
140+
To resolve, you can either start over by using `git reset --hard <backup-branch>`
141+
to return to the pre-rebase state, or you can create an additional commit to
142+
manually correct the rebase errors.

‎docs/operations/git/review_pr.md‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# Reviewing a Pull Request
2+
This document describes the process for reviewing a pull request (PR). Specific
3+
review criteria may apply to different projects, but this document aims to
4+
cover the general process that applies to Neon AI repositories.
5+
> See also [Project QA Standards](../qa_standards.md) for
6+
more information about expectations for testing and
7+
automation.
8+
9+
## When to Review
10+
A PR should generally be reviewed by requested reviewers when it is marked
11+
"Ready for Review". If a review is requested on a Draft PR, the reviewer should
12+
read the PR description and comments to understand if the author is looking for
13+
specific feedback or input.
14+
> Note that a PR may be marked "Ready for Review", but pending or failing
15+
checks may result in further changes before the PR is actually ready. In
16+
these cases, it is best to notify the PR author of the issues and hold a
17+
full review until they are resolved.
18+
19+
## Review Process
20+
The purpose of PR reviews is to ensure that changes are complete, described
21+
accurately and completely in the PR description, adequately tested, and that
22+
any relevant issues are linked and resolved or otherwise partially addressed.
23+
24+
### 1. Read the PR Title, Description, and Comments
25+
The PR title should be a succinct and understandable statement of the changes
26+
in the PR. If unclear, edit or ask the author to do so. The PR description
27+
should provide an understandable summary of the changes and highlight any
28+
specific trade-offs or design decisions. If any of those design decisions don't
29+
make sense or there is a better alternative, quote the relevant line in a
30+
comment to discuss with the PR author. Be sure to read the comment history as
31+
well in case the decision has already been discussed.
32+
33+
Once there is clear alignment on the goal of the PR and design discussions are
34+
resolved, the code is ready for review.
35+
36+
### 2. Review the Code Changes
37+
Code changes should be reviewed for correctness, readability, consistency, and
38+
scope. Much of the review process is subjective, so this document will only aim
39+
to provide guidance for things to look for, rather than a rubric for evaluation.
40+
41+
#### Correctness
42+
Changes should obviously be free of syntax errors and other bugs. Ensure that
43+
any changes come with test coverage or that they can be manually validated if
44+
adding tests is not feasible. Consider any edge cases or potential exceptions
45+
and confirm they are handled or at least acknowledged. If there are undefined
46+
behaviors that are not explicitly handled or discussed in the PR, flag them as
47+
issues to be resolved.
48+
49+
#### Readability
50+
Review new methods, functions, and variables for clear names and type annotations
51+
as appropriate. New methods and functions should have docstrings that match the
52+
format of other docstrings in the repository. If there is complex logic that
53+
isn't clear, make sure it has a comment explaining its purpose. Ensure that
54+
any exception handling includes clear logs and that any raised exceptions use
55+
an appropriate exception type and message.
56+
57+
#### Consistency
58+
Ensure that changes are consistent with the existing codebase. New functions and
59+
classes should integrate with existing modules where it makes sense and
60+
signatures should follow the same format as existing code. Suggest changes as
61+
appropriate to help the author address any issues.
62+
63+
#### Scope
64+
All changes in the PR should be constrained in scope to one specific feature,
65+
fix, documentation task, or other discrete change. If a PR includes multiple
66+
unrelated changes, flag the changes that are out of scope to request they be
67+
separated into their own PR(s). This can be particularly helpful if some
68+
changes are complete and ready to merge, while others have extensive feedback
69+
to address.
70+
> Note that this may lead to PRs that are dependent on each other. Use your
71+
judgement to determine if this is appropriate, or if separating changes
72+
will create more work for limited benefit.
73+
74+
### 3. Complete the Review
75+
Once changes have been reviewed and comments left, a review must be completed.
76+
A review can be completed to Approve, Request Changes, or Comment on the PR.
77+
78+
#### Approving Review
79+
If there are no changes to be made and the PR is ready to be merged, leave an
80+
approving review. This may be done with or without comment, but it is best
81+
practice to leave a comment indicating what you did (i.e. tested changes,
82+
reviewed code, reviewed tests, etc.) in case something comes up in the future
83+
and the merged PR needs to be referenced.
84+
> Note that an approving review may include comments that suggest a future
85+
enhancement or consideration to be moved to a new Issue. These should be
86+
addressed and resolved before merge, even though the PR is already approved.
87+
88+
#### Requesting Changes
89+
If there are issues noted in the review that must be addressed before the PR
90+
is ready to merge, leave a review requesting changes. It is helpful to highlight
91+
in the review comment what comments in particular need to be addressed and if
92+
any of the comments are optional suggestions or open for discussion.
93+
94+
#### Comment Only
95+
If there is feedback that is not a specific request for changes and the PR is
96+
not yet ready to be approved, a Comment review may be left. This will most
97+
often be done when a PR is still a Draft and comments don't necessarily relate
98+
to a specific change. This kind of review can also be helpful when changes are
99+
reviewed, but further testing will be done before approval.
100+
> If the PR author is reviewing changes, they are only allowed to leave a
101+
Comment, since it would not make sense to approve or request changes on their
102+
own PR. This can be a useful practice for reviewing changes before requesting
103+
reviews from others.
104+

‎mkdocs.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ nav:
138138
- Git:
139139
- Undo a Stable PR Workflow: operations/git/undo_stable_pr.md
140140
- Triage New GitHub Issues: operations/git/triage_issues.md
141+
- Development Workflow: operations/git/development_workflow.md
142+
- Pull Request Review: operations/git/review_pr.md
141143
- Skills:
142144
- Stable Release Workflow: operations/skills/workflow_stable_release.md
143145
- Alpha Release Workflow: operations/skills/workflow_alpha_release.md

0 commit comments

Comments
 (0)