Proposal for the API Review process

[terrajobst]

Now that we’re on GitHub we also get requests for new APIs. The current API review process is designed around the assumption that it is internal-only. We need to rethink this process for an open source world. This issue represents a proposal how this could be handled. It’s not final – any feedback is highly appreciated.

Process Goals

The key goals are:

• Designed for GitHub. In order to be sustainable and not be a hurdle for contributors the API review process must feel natural to folks familiar with GitHub.
• Efficiency. Performing API reviews requires looping in a set of experts. We want to conduct API reviews in an agile fashion without randomizing the reviewers or community members.
• Transparency. We can use the same process for both internal as well as external contributors. This allows contributors to benefit from the results of API reviews even if the implementer isn’t external.

Overall Process

GitHub is generally based around the pull-request model. The idea is that contributors perform their changes in their own fork and submit a pull request against our repository.

For trivial code changes, such as typo fixes, we want folks to directly submit a pull request rather than opening an issue. However, for bug fixes or feature work, we want contributors to first start a discussion by creating an issue.

For work that involves adding new APIs we'd like the issue to contain what we call a speclet. The speclet should provide a rough sketch of how the APIs are intended to be used, with sample code that shows typical scenarios. The goal isn't to be complete but rather to illustrate the direction so that readers can judge whether the proposal is sound.

Steps

• Contributor opens an issue. The issue description should contain a speclet that represents a sketch of the new APIs, including samples on how the APIs are being used. The goal isn’t to get a complete API list, but a good handle on how the new APIs would roughly look like and in what scenarios they are being used.
• Community discusses the proposal. If changes are necessary, the contributor is encouraged to edit the issue description. This allows folks joining later to understand the most recent proposal. To avoid confusion, the contributor should maintain a tiny change log, like a bolded “Updates:” followed by a bullet point list of the updates that were being made.
• Issue is tagged as “Accepting PRs”. Once the contributor and project owner agree on the overall shape and direction, the project owner tags the issue as “Accepting PRs”. The contributor should indicate whether they will be providing the PR or only contributed the idea.
• Coding. The contributor is implementing the APIs as discussed. Minor deviations are OK, but if during the implementation the design starts to take a major shift, the contributor is encouraged to go back to the issue and raise the concerns with the current proposal.
• Pull request is being created. Once the contributor believes the implementation is ready for review, she creates a pull request, referencing the issue created in the first step.
• Pull request is being reviewed. The community reviews the code for the pull request. The review should focus on the code changes and architecture – not the APIs themselves. Once at least two project owners give their OK, the PR is considered good to go.
• Pull is tagged as “Needs API Review”. The project owner then marks the pull request as “Needs API Review”.
• API review. Using the information in the pull request we’ll create an APIX file that constitutes the API delta. The API review board meets multiple times a week to review all PRs that are tagged as needing an API review.
• Pull request is updated with the results of the API Review. Once the API review is complete, the project owner uploads the notes and API HTML diff, including all comments. The project owner also updates the PR accordingly, with either a call to action to address some concerns or a good to go indicator.
• Pull request is merged. When there are no issues – or the issues were addressed by the contributor, the PR is merged.

API Design Guidelines

The .NET design guidelines are captured in the famous book Framework Design Guidelines by Krzysztof Cwalina and Brad Abrams.

A digest with the most important guidelines are available in our developer wiki. Long term, we'd like to publish the individual guidelines in standalone repo on which we can also accept PRs and -- more importantly for API reviews -- link to.

[justinvp]

An example of a good speclet would be useful. Other than that, LGTM.

[sharwell]

My initial comments:

1. You'll need to have a plan in place for cases where users submit code which constitutes an API change but no prior speclet was provided. Writing rules is relatively easy; the art with this plan is designing it in a way that guides users through the process without discarding work (unnecessarily) and without making the user feel like their input was unwelcome.
2. The Needs API Review, API Review, and Results of API Review are "a bit scary", but I certainly understand their importance. I would recommend the following if at all possible:
   • Have a personal goal among Microsoft employees contributing to this project to empower non-Microsoft community members to submit code which gets accepted following the API Review. During the Pull request is being reviewed phase, if you observe any component of the of the submission that may result in red flags for the review board, try to point them out (especially if they can be easily resolved).
   • Make sure the API Review board has line-item veto power. If a pull request makes 3 API changes, but only one is accepted, make sure to guide the author through the process of submitting a new restricted pull request that only includes the approved change.
   • Depending on the size of the review board, try to include at least one non-Microsoft employee who is particularly well-versed in enterprise application development, and one non-Microsoft employee who is particularly well-versed in open source development.
   • Find some way to index results coming from the API Review board for community members to read. The job performed by this board is relevant even to completely unrelated projects, and I believe insight into their activities will strengthen development practices in, around, and even apart from Microsoft's recently released projects.

[terrajobst]

An example of a good speclet would be useful.

That's an excellent suggestion. My psychic powers tell me you've some prior experience in this area ;-) Would you be willing to provide an example for your suggestion #13933?

You'll need to have a plan in place for cases where users submit code which constitutes an API change but no prior speclet was provided.

Fully agreed. My thinking was that we'd publish the process in our wiki, link to good examples, and then simply point people to it in case they haven't done it.

[API reviews] are "a bit scary"

That's a reaction we always got, including from internal folks.

We don't think of the API review board as the framework police. API design is an art and as such there usually isn't a clear cut right or wrong answer. The board is more like a group of experts that help providing a lot of insight into how non-experts would be able to use the APIs and how they fit in the larger picture, given the existing API patterns that exist.

Also, it may be worth pointing out that the API review board is the same set of people that are behind .NET Core and this GitHub project. So it's not like there is this super secret team in Microsoft :-)

Have a personal goal among Microsoft employees contributing to this project to empower non-Microsoft community members to submit code which gets accepted following the API Review.

Absolutely. The very idea of having an open development process is to empower contributors to be successful. In other words: it's the goal of this process :-)

Make sure the API Review board has line-item veto power

That's exactly how we operate. API review feedback is virtually always like don't do this, do this, consider this etc.

[Structure of the review board]

We don't want to change the structure of our review board yet. We don't want to rock the boat, we want to change one piece at a time to make sure it works before we change the next piece. In other words: we want to start by exposing the API review process itself.

Find some way to index results coming from the API Review board for community members to read.

That's a interesting point; being able to search would be great. I'm currently thinking of publishing the notes and API diffs in a separate GitHub repo. We could probably use a Wiki sot that we can use a full text search. But we're open for suggestions, so please let us know if you have a better idea.

[scalablecory]

• A sample "speclet" would be very useful. Is this merely a short code block within an issue to describe usage, or is it something more?
• I can think of cases for splitting issues with particularly large surface areas into multiple pull requests, if the issue itself can't be reasonably split.
• It sounds like the API Review team will be a bottleneck in the process -- the simple act of review will become a job in of itself. Finding a way to pull in the community here would be great.
• How are changes which are less a controversial addition and more of a patch to bring existing APIs into feature parity to be treated? (Speaking directly of Add async document/element loading for XLinq. corefx#110 as an example). In these cases an issue-style review process may take longer than simply diving into the patch itself.

[sharwell]

That's a reaction we always got, including from internal folks. ... We don't think of the API review board as the framework police. ...

I agree here. My comment was mostly regarding the wording/presentation. Even if the only thing you actually add to the proposal itself is a "We're here to help you out. If you have questions or concerns, please don't hesitate to ask.", it will go a long way towards engaging people who are uneasy about contributing to the project.

Absolutely. The very idea of having an open development process is to empower contributors to be successful. In other words: it's the goal of this process :-)

Just remember you are fighting a long history of this not being the case; you're going to have to point this out many times, for quite a while into the future I suspect. It will help to be ready with stats about number of community requests you've merged in. Having seen some early numbers, you should know they really build your case.

Edit: You don't have to give stats in the proposal. See the first paragraph of this comment for what I'm looking for in the proposal itself.

[sharwell]

That's exactly how we operate. API review feedback is virtually always like don't do this, do this, consider this etc.

In addition to the speclet sample, it would be helpful to link from the Steps section to a page with example "output" from the review board for something that failed, something that partially succeeded, and something that fully succeeded.

[justinvp]

Would you be willing to provide an example for your suggestion #13933?

I updated the description for #13933 to be more of a speclet. I'm sure it can be improved. Not sure if it's what you want as a canonical example.

[n8ohu]

@terrajobst, this looks very good; one thing I was curious about was is there a preferred naming convention for community contributions of features that we would like to see that are not simply parts of existing frameworks? For example, a project I am working on uses a C# wrapper around a cross-platform C library and it would, in my opinion, be beneficial to other developers if it was part of the core; something similar would be desirable for cross-platform UI work as well, even if the majority of the comments on the .NET Framework team blog asking for WF/WPF seem to indicate otherwise.

[richlander]

@n8ohu - We are working on a separate (but related) plan for valuable libraries that are not appropriate for corefx or have not been proven to be stable (in terms of API shape). This could be the answer for your issue.

This is a separate topic to the main one being discussed, so I've forked the conversation into issue dotnet/corefx#307. Please share your thoughts there.

[terrajobst]

@scalablecory

A sample "speclet" would be very useful. Is this merely a short code block within an issue to describe usage, or is it something more?

The speclet is simply a structured issue description. @justinvp did a good job with issue #13933. We'll provide more detail there. We currently don't have good examples; I think part of the process is to create a few good examples and later on point to them from our dev wiki.

I can think of cases for splitting issues with particularly large surface areas into multiple pull requests, if the issue itself can't be reasonably split.

Yes. I can think of cases where one issue is the uber-issue that represents the feature set. It could then link to specific issues that are actionable and can be realized by a single PR.

It sounds like the API Review team will be a bottleneck in the process -- the simple act of review will become a job in of itself.

We'll see. Internally, API reviews are designed not to bottleneck folks. It's just another thing that we have to do before we can ship. For GitHub, the act of shipping is essentially equivalent of being merged into master. We strongly believe in the value that API reviews provide so we're willing to risk moving a bit slower than we could.

How are changes which are less a controversial addition and more of a patch to bring existing APIs into feature parity to be treated?

We don't differentiate between trivial API changes and complicate API changes. It's the same as with code reviews: we review every change to the product, but the review itself might be super quick.

In these cases an issue-style review process may take longer than simply diving into the patch itself.

That's true and we talked about that. We currently settled on the approach that every PR that requires a public API warrants a dedicated issue. We may streamline this process later once we've more experience with how things work in the open.

[terrajobst]

@sharwell:

In addition to the speclet sample, it would be helpful to link from the Steps section to a page with example "output" from the review board for something that failed, something that partially succeeded, and something that fully succeeded.

That's a good suggestion. Similar to the speclet, my goal would be that we'll use the first GitHub API reviews as those examples.

[eatdrinksleepcode]

I have to start by saying that as a .NET developer for more than 11 years, I am ecstatic that we are even having this conversation :)

I like the overall direction of the process as outlined. I have a couple of comments:

1. I strongly agree with goal _#_3: Transparency. It is important to the health of the open source community that non-Microsoft contributors not feel like they are second-class citizens.

2. As it stands now, the API Review is the last part of the process. For some (especially larger) changes it might be helpful for the API Review to happen before the effort has been made to fully implement the feature, so that there is less rework required.

3. Like several others, I am concerned about the API Review process being a bottleneck; however, I understand the value that it is intended to bring, and based on the description from those who are familiar with it, I think having it is reasonable to start out with, with the understanding that we will frequently retrospect on this process and adjust it if necessary. My bigger concern is that, based on the limited information here, the API Review board seems to be a "closed-door" process: the board meets to discuss potential contributions without the presence of the contributor(s). There is no opportunity for the board to ask the contributor questions, or for the contributor to explain their decisions. I believe such conversations are critical both to the quality of new designs and to health of the open source ecosystem that we are all hoping will grow around this effort.

[terrajobst]

@eatdrinksleepcode:

I am ecstatic that we are even having this conversation :)

Glad to hear that!

As it stands now, the API Review is the last part of the process.

Our intent is to front-load as much work as possible while not doing big design up front. That's why we're asking for a speclet first, so that we can get consensus on what it is that you want to do, i.e. the big picture. The API review is designed to do a thorough pass to find the long tail of tiny issues here and there. However, this shouldn't result in a rewrite. If it does, then either the speclet wasn't good at capturing the controversial aspect or we didn't do a good job of giving feedback. Either outcome would be a learning for both, the community and us. It will likely take as few iterations to become good at this.

There is no opportunity for the board to ask the contributor questions, or for the contributor to explain their decisions.

While the review board meeting is certainly happening outside of GitHub, our intent is to be as transparent as possible. On campus, we invite the author of an API and found this invaluable. However, on campus it's obviously easier because we're on site and in the same time zone.

Currently, our goal is to compensate for the lack of involving the author in the API review process by asking the right questions when we review the speclet so that we can represent the author's intent during the review. It's certainly not perfect but probably a good enough approximation. If we find it doesn't work, we'll need to re-adjust.

[eatdrinksleepcode]

Thanks @terrajobst. I think I am not entirely understanding the separation between what the review board (which as I understand it is made up of people who are also contributors to the CoreFX repo and are communicating on this forum) will do, and what will happen prior to getting to the review board. Is the review board merely an official sign off for things that should have already happened on the issue/PR ahead of time? Perhaps the best thing is for me to just watch the process happen and I can try to pick up the nuance that way.

[terrajobst]

The API review process is designed to focus on the APIs. We look at the API diff and at sample code using the APIs. Passing API review is essentially a sign-off, yes. But it's also designed to do the thorough pass as I mentioned.

For open source, I'd hope that we can address ~60% of what the API review process does on GitHub. Internally, we'll often get only involved after the feature is implemented. That means we often see the APIs and the scenarios for the first time. On GitHub, this should be different because we already took a stab at the speclet as well as the PR review.

I know that this is a bit hand wavy at this point, but that's because we never did API reviews on open source. So I think we just need to do a few reviews to see what it looks like and how it feels for all parties involved.

Fair?

[eatdrinksleepcode]

Fair enough :)

[bbowyersmyth]

How does documentation like the MSDN Library get updated from an API change? Is Microsoft handling that or does it need to be a step in this process?

[alguar]

Unferstood

Sent from my Windows Phone

---

From: Immo Landwerthmailto:[email protected]
Sent: ‎12/‎23/‎2014 9:19 PM
To: dotnet/corefxmailto:[email protected]
Subject: Re: [corefx] Proposal for the API Review process (#294)

The API review process is designed to focus on the APIs. We look at the API diff and at sample code using the APIs. Passing API reviews is essentially a sign-off, yes. But it's also designed to do the thorough pass as I mentioned.

For open source, I'd hope that we can address ~60% of what the API review process does on GitHub. Internally, we'll often get only involved after the feature is implemented. That means we often see the APIs and the scenarios for the first time. On GitHub, this should be different because we already took a stab at the speclet as well as the PR review.

I know that this is a bit hand wavy at this point, but that's because we never did API reviews on open source. So I think we just need to do a few reviews to see what it looks like and how it feels for all parties involved.

Fair?

---

Reply to this email directly or view it on GitHub:
https://github.com/dotnet/corefx/issues/294#issuecomment-68022514

[terrajobst]

@bbowyersmyth

How does documentation like the MSDN Library get updated from an API change? Is Microsoft handling that or does it need to be a step in this process?

Same as today, we'll update the MSDN documentation. We haven't defined the cadence yet; it's most certainly not instantaneous. We'll probably update them similar to how we'd update what we call distributions our meta packages, which is expected to be a few times a year (> 1, < 5).

[justinvp]

There is no opportunity for the board to ask the contributor questions, or for the contributor to explain their decisions.

While the review board meeting is certainly happening outside of GitHub, our intent is to be as transparent as possible. On campus, we invite the author of an API and found this invaluable. However, on campus it's obviously easier because we're on site and in the same time zone.

Currently, our goal is to compensate for the lack of involving the author in the API review process by asking the right questions when we review the speclet so that we can represent the author's intent during the review. It's certainly not perfect but probably a good enough approximation. If we find it doesn't work, we'll need to re-adjust.

The ASP.NET team has been doing Community Standup meetings using Google Hangouts. Worth considering doing something similar for the API reviews.

[sharwell]

The ASP.NET team has been doing Community Standup meetings using Google Hangouts. Worth considering doing something similar for the API reviews.

It would be awesome to use a shared broadcast (open for viewing but not public feedback) during API reviews, especially for API contributions submitted by non-Microsoft individuals. Even for people that aren't working on the API change itself, observing the process could be valuable as a learning experience for teams that want to become the most effective .NET developers they are can.

[terrajobst]

Thanks for all the feedback!

• I've added the API review process to our wiki
• Today, we've also uploaded our first set of notes from an API review meeting we did today.
• We plan on conducting an API review four our GitHub issues next week. I try to get it recorded, stay tuned.

[justinvp]

I try to get it recorded, stay tuned.

The ASP.NET Community Standup meetings are live (as well as recorded). It'd be really cool if the API Reviews were live too. And if the API contributor was able to watch live, he/she could answer any questions or provide clarifications immediately in the chat room during the review.

[terrajobst]

I was indeed able to record and you can find the recording here.

[eatdrinksleepcode]

I really appreciate the obvious effort that went into preparing and publishing the notes and video from the API Review. The notes were clear and helpful, and the direct links to the correct place in the video for the discussion of each PR was a nice touch. The video itself was very informative, not to mention utterly fascinating. I am sure it was business as usual for you guys, but as a long-time .NET developer who has always been curious about how these discussions happen inside DevDiv, I loved watching it. I also appreciate that there is a certain amount of risk in opening your internal conversations up to the world like that; I for one found it very valuable, and look forward to future episodes :)

[terrajobst]

@eatdrinksleepcode, thanks a lot for the kind words and for taking the time giving us positive feedback! This is highly appreciated.

[RichiCoder1]

👍

[slang25]

I agree with @eatdrinksleepcode, I found this a fascinating insight into the decisions you have to make. It must be extremely difficult having to maintain such strict backwards compatibility.

[hickford]

I've added the API review process to our wiki

The link is dead. Where should I look?

Update: Found it https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[hickford]

A question about API review process described at https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

If a proposal is approved, in which frameworks would the API be implemented? .NET Core? .NET Framework? Both (.NET Standard)?