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.

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

[DOCS][COMMUNITY] Improve code review guideline on API designs #2459

Merged
merged 2 commits into from
Jan 18, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions docs/contribute/code_review.rst
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,37 @@ Open source code is maintained by a community with diverse backend, and it is ev
Here are some checklists for code reviews, it is also helpful reference for contributors



Hold the Highest Standard
-------------------------
The first rule for code reviewers is to always keep the highest standard, and do not approve code just to "be friendly". Good, informative critics each other learn and prevents technical debt in early stages.

Deliberate on API and Data Structures
-------------------------------------
A minimum and stable API is critical to the project’s life. A good API makes a huge difference. Always think very carefully about all the aspects including naming, argument definitions and behavior.

When possible, pay more time and thoughts into the API design during code reviews.
Remember, it is easier to improve code implementation, but it is extremely hard to change an API.
We should do the same for data structures that are shared across modules(e.g. AST).
When uncertain, start a conversation with more developers.

Here are some useful principles for designing APIs:

- Be consistent with existing well-known package’s APIs if the feature overlap.
For example, tensor operation APIs should always be consistent with the numpy API.
- Be consistent with existing APIs in the same project.
For example, we should use the same argument ordering across all the optimization passes,
so there is no "surprise" when using them.
- Think about whether the API will change in the future.
For example, we will have more options like loop_unrolling and device placement policy
as we add more optimizations in build. We can package optimization knobs into a build
configuration object. So that the build API is stable over time.
- Write down documents. Documents are mandatory for APIs and sometimes writing documents helps
us to think about whether we need clarification.
- Minimum. Think about how many lines of code a user has to write to use the API.
Remove layers of abstraction when possible.


Ensure Test Coverage
--------------------
Each new change of features should introduce test cases, bug fixes should include regression tests that prevent the problem from happening again.
Expand All @@ -20,10 +47,6 @@ Documentations are Mandatory
----------------------------
Documentation is usually a place we overlooked, new functions or change to a function should be directly updated in documents. A new feature is meaningless without documentation to make it accessible. See more at :ref:`doc_guide`

Deliberate on User-facing API
-----------------------------
A good, minimum and stable API is critical to the project’s life. A good API makes a huge difference. Always think very carefully about all the aspects including naming, arguments definitions and behavior. One good rule to check is to be consistent with existing well-known package’s APIs if the feature overlap. For example, tensor operation APIs should always be consistent with the numpy.

Minimum Dependency
------------------
Always be cautious in introducing dependencies. While it is important to reuse code and not reinventing the wheel, dependencies can increase burden of users in deployment. A good design principle only depends on the part when a user actually use it.
Expand Down