[DOCS][COMMUNITY] Improve code review guideline on API designs #2459
Conversation
This file contains 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
ZihengJiang
approved these changes
Jan 18, 2019
eqy
suggested changes
Jan 18, 2019
docs/contribute/code_review.rst
Outdated
| 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, arguments definitions and behavior. |
docs/contribute/code_review.rst
Outdated
| 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. |
docs/contribute/code_review.rst
Outdated
| - 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 API will change in the future. |
docs/contribute/code_review.rst
Outdated
| 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 API will change in the future. | ||
| For example, we will have more options like loop_unrolling, device placement policy |
There was a problem hiding this comment.
loop unrolling [and] device placement policy
docs/contribute/code_review.rst
Outdated
| so there is no "surprise" when using them. | ||
| - Think about whether API will change in the future. | ||
| For example, we will have more options like loop_unrolling, device placement policy | ||
| as we add more optimization in build. We can package optimization knobs into a build |
docs/contribute/code_review.rst
Outdated
| For example, we will have more options like loop_unrolling, device placement policy | ||
| as we add more optimization 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, sometimes writing documents helps |
docs/contribute/code_review.rst
Outdated
| configuration object. So that the build API is stable over time. | ||
| - Write down documents. Documents are mandatory for APIs, sometimes writing documents helps | ||
| us to think about whether we need clarification. | ||
| - Minimum. Think about how many lines of code a user have to write to use the API. |
eqy
approved these changes
Jan 18, 2019
|
Thanks to @eqy @ZihengJiang , this is now merged |
Anthony-Mai
pushed a commit
to Anthony-Mai/tvm
that referenced
this pull request
Jan 21, 2019
Closed
wweic
pushed a commit
to neo-ai/tvm
that referenced
this pull request
Feb 20, 2019
wweic
pushed a commit
to neo-ai/tvm
that referenced
this pull request
Feb 20, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Update code review guidelines to emphasize more on API designs. API design is what we sometimes overlooked, and we should learn from projects like numpy, pytorch and keras.
cc @dmlc/tvm-comitter