Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Docs: Clarify when an RFC can be merged #8

Merged
merged 7 commits into from
Jan 29, 2019
Merged

Docs: Clarify when an RFC can be merged #8

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4e983a9
Docs: Clarify when an RFC can be merged
nzakas Jan 7, 2019
afcb628
Updated proposal
nzakas Jan 14, 2019
82525c2
Add clarifications about commenting periods
nzakas Jan 15, 2019
5610da5
Fix typo
nzakas Jan 16, 2019
5ab5b44
Update initial commenting period
nzakas Jan 20, 2019
aaef578
More edits
nzakas Jan 25, 2019
043282b
Fix typo
nzakas Jan 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,14 @@ implementation for review after the RFC has been accepted.
When a pull request has implemented an RFC, the RFC should be updated with a link
to the PR implementing it.

## Merging an RFC

An RFC may be merged by a TSC member provided that:

1. Every TSC member who wishes to comment on the RFC has done so.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to evaluate this condition in practice. For example, if I want to merge an RFC, how would I determine if everyone who wishes to comment has done so? It seems like it would be unclear whether (a) someone hasn't commented because they're busy but plans to comment at some point, or (b) someone doesn't wish to comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that does seem fuzzy in retrospect. Maybe this should be replaced with some time period? Like an RFC must have an initial comment period of n weeks, which would give everyone enough time to comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience with #3 was:

  • Around the time the PR was opened, I made some comments but didn't have a strong opinion either way on whether it should be merged.
  • Later, a few team members discussed certain aspects of the RFC and I think some changes were made. I didn't take the time to understand these discussions while they were going on (since it seemed to be changing somewhat quickly)
  • I would have liked to have a chance to look at the final version after all changes were made, just to double-check that nothing was added that I would have a strong objection to. However, I wasn't sure if more changes were forthcoming (since I hadn't been following the discussions closely), so I put off looking it over. After a bit of time, the PR was merged and I hadn't ended up looking it over. Of course, given the number of approvals and the time elapsed, it's understandable that the PR was merged, since I hadn't mentioned I wanted to review the final version. I think the main issue in this case was that the PR had changed and I wasn't sure if it was about to change again (without reading through all the discussions).

Having a comment period is a good idea. I would slightly prefer a final comment period (with a length of a week, say) rather than an initial comment period. Entering the final comment period would imply that the PR has sustained some feedback, and isn't expected to need further changes barring a major objection.

If the design is changed as a result of objections during the final comment period, the timer would be reset to make sure everyone is still okay with those changes. (This does have a downside of potentially making the process take a bit longer.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we ping the TSC with a set amount of time to respond when the RFC is in a complete state and any concerns that have been brought up have been addressed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we're trying to avoid pinging teams in GitHub, but because it seems unlikely there will be many concurrent RFCs going on, it feels to me like it could be a good use of that feature.

1. All outstanding concerns and comments have been addressed.
1. Consensus has been reached on merging the RFC (no TSC member dissents to merging). This may be done on the RFC pull request by TSC members marking the pull request as approved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, does this also require that a quorum of TSC members have approved the PR (in addition to having no dissenters)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'd envision this as the same process we follow for core changes.


If an RFC fails to reach approval on the pull request then the RFC can be discussed during a TSC meeting to determine whether or not to merge.

**Thanks to the [Ember RFC process](https://github.com/emberjs/rfcs) for the inspiration for ESLint's RFC process.**