-
Notifications
You must be signed in to change notification settings - Fork 92
Feature review process #1563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
BillWagner
wants to merge
3
commits into
dotnet:draft-v9
Choose a base branch
from
BillWagner:Feature-review-process
base: draft-v9
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Feature review process #1563
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,41 @@ | ||||||
| # Feature PR workflow process | ||||||
|
|
||||||
| The purpose of this PR workflow process is to focus on the right review at the right time for each feature. We need enough detail in the feature PR to provide focused feedback, but we also want to keep the discussion focused on the decisions to drive the PR forward. | ||||||
|
|
||||||
| The goal is to provide two sets of gates through the review process: First, agree on the overall structure and edits necessary to specify the new feature. Second, refine the spec updates and finalize before merging. | ||||||
|
|
||||||
| ## Assignee review and update | ||||||
|
|
||||||
| A committee member takes ownership of the draft PR and self-assigns it. The *assignee* reviews and modifies the initial PR. At the assignee's discretion, they can tag other members for help or opinion on the overall direction of the PR. | ||||||
|
|
||||||
| Once the assignee has a good first draft, it's ready for the [First Reading](#first-reading). As part of getting ready for the first reading, the assignee should provide a summary of the PR to guide reviewers. This should cover: | ||||||
|
|
||||||
| - Roadmap of the PR. | ||||||
| - Open questions on how to spec the behavior. | ||||||
| - Comments that explain decisions on spec language. | ||||||
|
|
||||||
| ## First reading | ||||||
|
|
||||||
| The *first reading* is when the committee as a whole discusses the overall direction of the PR. This should focus on large-scale direction and concepts rather than nits in the specification language. (Although we likely can't help ourselves from pointing out the smaller issues we find as we read the PR.) | ||||||
|
|
||||||
| The assignee leads the discussion based on the PR summary and comments made by other committee members prior to the meeting. | ||||||
|
|
||||||
| The outcome of the first reading is one of: | ||||||
|
|
||||||
| - Direction approved, move on to final edits and [Second reading](#second-reading). | ||||||
| - Direction or content needs major work. Bring a revised PR back for a new first reading. (Task list comes from meeting review comments.) | ||||||
|
|
||||||
| The goal for this review is to minimize major rewrites to large new issues discovered later in the process. On rare occasion that a feature PR is quite small, the first reading and second reading can be combined. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| ## Second reading | ||||||
|
|
||||||
| The assignee addresses concerns raised during the first reading, and brings the PR to the next meeting for a second reading and final review. At this point cross references and style should be addressed. | ||||||
|
|
||||||
| The outcome of the second reading is one of: | ||||||
|
|
||||||
| - Merge after applying suggestions. | ||||||
| - Agree to merge offline after other edits. | ||||||
|
|
||||||
| In addition, in the case of major issues identified only at the second reading, the committee agrees to open issues for outstanding work related to the feature. | ||||||
|
|
||||||
| > The lack of a mechanism for a complete rejection of a feature PR at the second reading stage is intentional. The goal is to catch issues at that scale in the first reading. The plan is that major issues that slip through become issues to be addressed in the same release of the standard. | ||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably if the assignee thinks the direction is actually wrong, and wants overally committee feedback, that's fine to bring to a first reading too?