Skip to content

Commit

Permalink
Add API design process notes (#541)
Browse files Browse the repository at this point in the history 
* Round 1 - formatting enforcement

* Update

* Process note clarification

* PR feedback

* PR feedback

* Update docs/Coding-Guidelines.md

Co-authored-by: Raymond Chen <[email protected]>

* Update specs/README.md

Co-authored-by: Raymond Chen <[email protected]>

* PR feedback

Co-authored-by: Raymond Chen <[email protected]>
  • Loading branch information
@jonwis @oldnewthing
jonwis and oldnewthing authored Mar 11, 2021
1 parent 91dc45c commit e582728
Show file tree
Hide file tree
Showing 4 changed files with 191 additions and 68 deletions.
6 changes: 6 additions & 0 deletions docs/.prettierrc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"tabWidth": 4,
"printWidth": 100,
"proseWrap": "always",
"useTabs": false
}
59 changes: 42 additions & 17 deletions docs/Coding-Guidelines.md
View file
Original file line number Diff line number Diff line change
@@ -1,34 +1,59 @@
#### General
# General

Project Reunion prefers using industry-standard coding styles, guidelines, and patterns for any languages used in implementation or testing.
Project Reunion prefers using industry-standard coding styles, guidelines, and patterns for any
languages used in implementation or testing.

#### C++
## Languages

Project Reunion implementations prefer C++ and C++/WinRT implementations. New code must adhere to the **cppcoreguidelines** at
https://github.com/isocpp/CppCoreGuidelines and be /W4 clean (for Visual C++.)
This list is non-exhaustive; new guidelines for languages will be added over time.

Prefer existing std:: and gsl:: types, but use **wil** (https://github.com/Microsoft/wil) types for Windows-specific helpers and lifecycle management
rather than creating your own.
### C++

##### Catching Exceptions and HRESULT
**DO** implement types using C++ and [C++/WinRT](https://github.com/microsoft/cppwinrt)

Don't just catch `winrt::hresult_error` or other variations as that doesn't catch `std::bad_alloc`.
**DO** follow the [CppCoreGudelines](https://github.com/isocpp/CppCoreGuidelines) for all new code.

Use the following snippet to catch exceptions and retrieve their HRESULT:
**DO** enable `/W4 /Wx` for all new code.

**CONSIDER** using the [Windows Implementation Library](https://github.com/Microsoft/wil) for
Windows-specific helpers rather than creating your own.

**DO** use 4-space indentation instead of tab characters.

##### Catching C++/WinRT Exceptions and HRESULT

Exceptions should not be used for standard flow control.

When a catch clause is required, note that `winrt::hresult_error` does not also catch
`std::bad_alloc` or any of the other C++ standard exceptions. In cases where a single catch handler
desires to pull out an HRESULT, use the following:

```c++
// For code using C++/WinRT
catch (...)
{
auto e{ winrt::hresult_error(winrt::to_hresult(), winrt::take_ownership_from_abi) };
...e.code() contains the WinRT exception or best-guess conversion from a C++ exception...
auto hr{ e.code() };
auto message { winrt::to_message() };
// hr contains the WinRT exception or best-guess conversion from a C++ exception,
// message contains the best-guess textual format of that exception
}
```

#### Markdown
// For code using WIL
catch (...)
{
auto hr{ wil::ResultFromCaughtException() };
// hr contains the WinRT exception, WIL exception, or best-guess conversion from a C++ exception
}
```

**GUIDELINE:** The preferred line length limit is ~100 characters. GitHub formats lines regardless of individual length but GitHub diff is line oriented.
Keeping lines within the preferred limit makes changes easier to review.
C++/WinRT's ABI layer automatically catches and converts exceptions generated during a method call
and uses a similar mechanism to map a C++ exception to an `HRESULT` that can be delivered across the
ABI.

#### Other Languages
### Markdown

If new languages become common, we will describe the coding guidelines for such languages here.
**DO** wrap lines at ~100 characters. GitHub formats lines regardless of individual length but
GitHub diff is line oriented. Keeping lines within the preferred limit makes changes easier to
review. Use a tool like Prettier to bulk-reformat files, or configure your editor's "rulers." If new
languages become common, we will describe the coding guidelines for such languages here.
6 changes: 6 additions & 0 deletions specs/.prettierrc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"tabWidth": 4,
"printWidth": 100,
"proseWrap": "always",
"useTabs": false
}
188 changes: 137 additions & 51 deletions specs/README.md
View file
Original file line number Diff line number Diff line change
@@ -1,84 +1,170 @@
# Project Reunion Specs

This directory contains archived and in-progress spec documents for APIs in
This directory contains archived and in-progress spec documents for APIs in Project Reunion.

Documents in this directory are used as part of a feature or API review for features in development
and form the basis for public documentation and sample code. Write specs so they can be directly
consumed by an app developer or document author.

**DO** provide feedback by **commenting on active pull requests** tagged with
[the "api-design" label.](https://github.com/microsoft/ProjectReunion/pulls?q=is%3Apr+is%3Aopen+label%3Aapi-design)

**DO** provide feedback by **opening
[issues](https://github.com/microsoft/ProjectReunion/issues/new/choose) regarding spec documents in
this folder**. Specs in this folder are ready for implementation or have already been added to
Project Reunion.

Documents in this directory are used as part of a feature or API review
for features in development and form the basis for public documentation
and sample code. Write specs so they can be directly consumed by an
app developer or document author.
**DO** create pull requests for new or updated specs when requested to do so as part of an issue
thread.

**DO NOT** create unsolicited PRs that add or modify specs. To propose a new feature please follow
the Project Reunion contribution process [described here](../docs/contributor-guide.md) and start by
[opening a new issue](https://github.com/microsoft/ProjectReunion/issues/new/choose).

## Please DO:
## API Design

* Provide feedback by **commenting on active pull requests** tagged with
[the "ApiReview" label.](https://github.com/microsoft/ProjectReunion/pulls?q=is%3Apr+is%3Aopen+label%3AApiReview)
* Provide feedback by **opening [issues](https://github.com/microsoft/ProjectReunion/issues/new/choose)
regarding spec documents in this folder**. Specs in this folder
are ready for implementation or have already been added to Project Reunion.
* Create pull requests for new or updated specs when requested to do so
as part of an issue thread.
Designing a new Project Reunion API involves creating a spec for the API, authoring some sample code
an app might use with the API, getting it reviewed by a group of other API designers, and finally
merging the spec into `main`.

## Please DO NOT:
| Who | Role |
| --------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Author | Member of the Project Reunion community proposing a new API or change to an existing API |
| Design Representative | Member of the Windows API Design Representatives group responsible for the shape and direction of their component area within Windows or Project Reunion |
| Reviewer | Person providing commentary on an API under review |

* Create unsolicited PRs that add or modify specs. To propose a new feature
please follow the Project Reunion contribution process [described here](../docs/contributor-guide.md)
and start by [opening a new issue](https://github.com/microsoft/ProjectReunion/issues/new/choose).
For additional help, create a new `api-design` tagged issue and assign it to @jonwis for assistance.

## Spec workflow
### Authoring an API Spec

For Project Reunion team members, the spec and API design workflow is:

1. Ensure your proposal is approved and in plan.

2. Create a new working branch ```user/<username>/<feature>``` or work in
your fork of the Project Reunion repo.
2. Create a new working branch `user/<username>/<feature>` or work in your fork of the Project
Reunion repo.

3. Create a new folder for your spec under the `specs` folder: `/specs/<feature>`

4. Author your spec using the [spec template](spec_template.md). Please use relative links for
images or other assets in the folder. Be sure to read and follow the commented instructions in
the template.

5. Work with your API design representative and iterate on the specification in your branch as much
as you see fit.

When your spec is ready for review, your API representative will help guide you through the
remainder.

**DO** follow the authoring guidelines from [the contributor's guide](../docs/contributor-guide.md).

**DO** follow the [spec template guidance](spec_template.md) for authoring samples and IDL.

**DO NOT** create a pull request from `user/<username>/<feature>` to `main` (see "Reviewing"
section).

### Reviewing APIs

Your API representative will help you finish the design document and get it reviewed with members of
the Windows API Design community. The process to finish the review is:

1. The representative creates a new pull request from `user/<username>/<feature>` to the `main`
branch. Make sure the pull request mentions the original issue thread.

2. The representative schedules time with the Windows API Design community to review the pull
request.

- As the design group is spread out we prefer giving no less than 48h notice when reviewing APIs
- Reviews are typically scheduled for Tuesdays 1-2pm or Thursdays 1-3pm (Seattle local time).
- API reviewers should post in the initial issue thread with the date and time of the review and
any group-call information.

3. The design group (and you, the community!) leaves comments in the pull request

- Make sure to leave comments _before_ the scheduled time; comments afterwards may not be seen
or applied
- API authors should _avoid_ responding to posted comments until _after_ the review

4. During the review timeslot, the API design group will join a Teams call and review any comments
on the pull request. See below for the "feedback keywords" the review meeting uses.

> Note: community members are welcome to join our Teams call just as soon as we can figure out
> how to publish the Teams link for public joining.
5. After review, the API design representative works with the API designer to apply any feedback
generated during the review period.

6. When the API representative is satisfied that the spec is ready to go they can press the Merge
button on the pull request.

**DO** send deep-links to both the formatted and raw views of the markdown spec. To get the link
information:

- In the pull request UI, select the "Files Changed" tab
- Click the "Changes from nn commits" drop down
- Select the full range of commits to review
- Send the resulting pull-request URI
- Send a link to "source diff" of the markdown file in question
- Click the "show rich diff" button and send a resulting link for the formatted version

**AVOID** pushing updates to the under-review branch until the review has completed.

**DO NOT** click the "Approve" button when finishing your review commentary. Instead, use the
"Comment" or "Request Changes" buttons.

**DO NOT** resolve conversations until _after_ the review period.

3. Create a new folder for your spec under the ```specs``` folder: ```/specs/<feature>```
**DO** update the issue thread when the review has completed.

4. Author your spec using the [spec template](spec_template.md).
Please use relative links for images or other assets in the folder.
Be sure to read and follow the commented instructions in the template.
**DO** push updates to the under-review branch to apply feedback to the spec.

5. When your spec is ready for review:
### Review Keywords

* Open a new pull request to merge your spec to the ```master``` branch
* Reference the issue thread in the pull request to link them together
During API review the representative and members of the call may provide new guidance for the API
author to apply. There may also be discussion about why the API is shaped in a certain way, or how
it should work. Review feedback is added to the pull request as comments on existing comments.

6. Provide adequate time for the community and Project Reunion members to
review the API spec file.
The keywords are:

7. Review requested changes with Project Reunion members and the Windows
API Design representative responsible for the area.
| Keyword | Meaning |
| ---------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| DISCUSSION | Notes from any conversation about the comment, or notes on a discussion around this part of the API. Discussions often record why a certain design choice was made, interesting topics for the future, or explaining how this API intersects with another API. |
| RECOMMEND | The API group _strongly recommends_ making a specific change to the API shape. Typical feedback includes naming, changes to type signatures, or modifications to behaviors or documentation. |
| CONSIDER | The reviewers ask that the author _consider_ making a specific change to the API shape, adding functionality, modifying samples or documentation, etc. |
| SAMPLE | A sample defect, missing sample, or other problem with the presented sample code. "Sample" tags typically do not modify the API shape itself |

8. Complete the pull request
The API design representative owns the quality and shape of the APIs for their area. Representatives
are free to ignore or modify RECOMMEND markers, but should reply back to the comment thread
indicating their reasoning. We use review commentary to adjust our design guidelines over time.

This workflow applies to modifying API designs as well.
### Additional Links

- [.NET API Design](https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md)
- [MIDL 3 Specification](https://docs.microsoft.com/en-us/uwp/midl-3/)

## Specification Notes

**DO** write specification templates as if they will be read by a
developer trying to understand how to use the API and feature.
**DO** write specification templates as if they will be read by a developer trying to understand how
to use the API and feature.

**DO** include links to existing types, pointers into https://docs.microsoft.com
when providing converged features.
**DO** include links to existing types, pointers into https://docs.microsoft.com when providing
converged features.

**DO** describe how packaged, unpackaged, AppContainer and Full Trust
applications will use the feature.
**DO** describe how packaged, unpackaged, AppContainer and Full Trust applications will use the
feature.

**DO NOT** reference Windows-internal features, plans, or scheduling. This
includes links to internal specifications.
**DO NOT** reference Windows-internal features, plans, or scheduling. This includes links to
internal specifications.

**DO** use "http://task.ms/12345" links if necessary to reference internal
tasks or deliverable information.
**DO** use "https://task.ms/12345" links if necessary to reference internal tasks or deliverable
information.

**DO** update existing specifications when adding new functionality,
rather than creating a new-spec-per-change. Reviewers should read the
changes to the specification in a pull request.
**DO** update existing specifications when adding new functionality, rather than creating a
new-spec-per-change. Reviewers should read the changes to the specification in a pull request.

**DO NOT** gratuitously reformat specification documents, as that can
obscure real changes vs editorial, whitespace, or other changes.
**DO NOT** gratuitously reformat specification documents, as that can obscure real changes vs
editorial, whitespace, or other changes.

**DO** specify any additional requirements on application authors, such
as "you must define an app.config" or "you must include this package
manifest markup or this application manifest markup" to use a feature.
**DO** specify any additional requirements on application authors, such as "you must define an
app.config" or "you must include this package manifest markup or this application manifest markup"
to use a feature.

0 comments on commit e582728

Please sign in to comment.