forked from facebookincubator/velox
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Revamp CONTRIBUTING.md guide (facebookincubator#5810)
Summary: Refactoring and adding more context to the CONTRIBUTING guide to reflect the current guidelines. Pull Request resolved: facebookincubator#5810 Reviewed By: kagamiori Differential Revision: D47887822 Pulled By: pedroerp fbshipit-source-id: 28890f61814051dbdc2bd1ecadc117f0738e4f17
- Loading branch information
1 parent
4096ae8
commit 9aeac4b
Showing
1 changed file
with
146 additions
and
63 deletions.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,111 +1,194 @@ | ||
| # Contributing | ||
|
|
||
| Welcome! Thank you for your interest in the Velox project. Before starting to | ||
| contribute, please, take a moment to review the contribution guidelines | ||
| outlined below. | ||
| contribute, please take a moment to review the guidelines outlined below. | ||
|
|
||
| Contributions are not just about code. Contributing code is great, but that’s | ||
| probably not the best place to start. There are lots of ways to make valuable | ||
| contributions to the project and community. | ||
| probably not the best place to start. There are many ways in which people can | ||
| make contributions to the project and community. | ||
|
|
||
| ## Code of Conduct | ||
|
|
||
| This project and everyone participating in it is governed by a [Code of Conduct](CODE_OF_CONDUCT.md). | ||
| By participating, you are expected to uphold this code. | ||
| First and foremost, the Velox project and all its contributors and maintainers | ||
| are governed by a [Code of Conduct](CODE_OF_CONDUCT.md). When participating, | ||
| you are expected to uphold this code. | ||
|
|
||
| ## Community | ||
|
|
||
| A good first step to getting involved in the Velox project is to join the | ||
| conversations in GitHub Issues and Discussions. | ||
| A good first step to getting involved in the Velox project is to participate in | ||
| conversations in GitHub [Issues](https://github.com/facebookincubator/velox/issues) | ||
| and [Discussions](https://github.com/facebookincubator/velox/discussions), and join the | ||
| [the Velox-OSS Slack workspace](http://velox-oss.slack.com) - please reach out to | ||
| **[email protected]** to get access. | ||
|
|
||
| ## Documentation | ||
|
|
||
| Help the community understand how to use the Velox library by proposing | ||
| additions to our [docs](https://facebookincubator.github.io/velox/index.html) or pointing | ||
| out outdated or missing pieces. | ||
|
|
||
| ## Bug Reports | ||
|
|
||
| Found a bug? Help us by filing an issue on GitHub. | ||
|
|
||
| Ensure the bug was not already reported by searching [GitHub Issues](https://github.com/facebookincubator/velox/issues). If you're | ||
| Ensure the bug was not already reported by searching | ||
| [GitHub Issues](https://github.com/facebookincubator/velox/issues). If you're | ||
| unable to find an open issue addressing the problem, open a new one. Be sure to | ||
| include a title and clear description, as much relevant information as | ||
| possible, and a code sample or an executable test case demonstrating the | ||
| expected behavior that is not occurring. | ||
| expected behavior. | ||
|
|
||
| Meta has a [bounty program](https://www.facebook.com/whitehat/) for the safe disclosure | ||
| of security bugs. In those cases, please go through the process outlined on that page | ||
| and do not file a public issue. | ||
|
|
||
| ## Documentation | ||
|
|
||
| Help the community understand how to use the Velox library by proposing | ||
| additions to our [docs](https://facebookincubator.github.io/velox/index.html) or pointing | ||
| out outdated or missing pieces. | ||
|
|
||
| ## Code | ||
| ## Code Contribution Process | ||
|
|
||
| This is the process we suggest for code contributions. This process is designed | ||
| to reduce the burden on project reviews, impact on other contributors, and to | ||
| keep the amount of rework from the contributor to a minimum. | ||
| The code contribution process is designed to reduce the burden on reviewers and | ||
| maintainers, allowing them to provide more timely feedback and keeping the | ||
| amount of rework from contributors to a minimum. | ||
|
|
||
| It is good to start with small bug fixes and tiny features to get familiar with | ||
| the contributing process and build relationships with the community members. | ||
| We encourage new contributors to start with bug fixes and small features so you | ||
| get familiar with the contributing process, while building relationships with | ||
| community members. | ||
| Look for GitHub issues labeled [good first issue](https://github.com/facebookincubator/velox/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22) or consider adding one of the | ||
| [missing Presto SQL functions](https://github.com/facebookincubator/velox/issues/2262). | ||
|
|
||
| 1. Sign the [Contributor License Agreement](https://code.facebook.com/cla) (CLA). This step needs to be completed only once. | ||
| The contribution process is outlined below: | ||
|
|
||
| 1. Review the [`LICENSE`](LICENSE) file. By contributing to Velox, you agree that your | ||
| 1. Sign the [Contributor License Agreement](https://code.facebook.com/cla) | ||
| (CLA). This step needs to be completed only once. | ||
|
|
||
| 2. Review the [`LICENSE`](LICENSE) file. By contributing to Velox, you agree that your | ||
| contributions will be licensed under that LICENSE file. This step needs to be | ||
| completed only once. | ||
|
|
||
| 1. Start a discussion by creating a Github issue, or asking on Slack (unless the change is trivial). | ||
| 3. Start a discussion by creating a Github Issue, or asking on Slack (unless the change is trivial). | ||
| * This step helps you identify possible collaborators and reviewers. | ||
| * Does the change align with technical vision and project values? | ||
| * Does the proposed change align with technical vision and project values? | ||
| * Will the change conflict with another change in progress? If so, work with others to minimize impact. | ||
| * Is this change large? If so, work with others to break into smaller steps. | ||
|
|
||
| 1. Review our coding style and best practices document in [`CODING_STYLE.md`](CODING_STYLE.md). | ||
|
|
||
| * Implement the change | ||
| * If the change is large, post a preview Github pull request with the title prefixed with [WIP], and share with collaborators. | ||
| * Include tests and documentation as necessary. | ||
|
|
||
| 1. Create a Github pull request (PR). | ||
| * Give the pull request a clear, brief description: when the pull request is merged, this will be retained in the extended commit message. Check out [How to Write Better Git Commit Messages – A Step-By-Step Guide](https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/) and [How to Write a Git Commit Message](https://cbea.ms/git-commit/) to learn more about how to write good commit messages. | ||
| * Make sure the pull request passes the tests in CircleCI. | ||
| * If known, request a review from an expert in the area changed. If unknown, ask for help on Slack. | ||
|
|
||
| 1. Review is performed by one or more reviewers. | ||
| * This normally happens within a few days, but may take longer if the change is large, complex, or if a critical reviewer is unavailable. (feel free to ping the pull request). | ||
| * Address concerns and update the pull request. | ||
|
|
||
| 1. After pushing the changes, add a comment to the pull-request, mentioning the | ||
| reviewers by name, stating the comments have been addressed. This is the only | ||
| way that a reviewer is notified that you are ready for the code to be reviewed | ||
| 4. Implement the change. | ||
| * Always follow the coding best practices outlined in the list below. | ||
| * If the change is large, consider posting a draft Github pull request (PR) | ||
| with the title prefixed with [WIP], and share with collaborators to get early feedback. | ||
| * Give the PR a clear, brief description; when the PR is | ||
| merged, this will be retained in the extended commit message. Check out | ||
| [How to Write Better Git Commit Messages – A Step-By-Step Guide](https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/) | ||
| and [How to Write a Git Commit Message](https://cbea.ms/git-commit/) to | ||
| learn more about how to write good commit messages. | ||
| * Make sure the PR passes all CI tests. | ||
| * Create/submit a Github PR and tag the reviewers identified in Step 3. | ||
|
|
||
| 5. Review is performed by one or more reviewers. | ||
| * This normally happens within a few days, but may take longer if the change is | ||
| large, complex, or if a critical reviewer is unavailable (feel free to ping in the | ||
| PR). | ||
|
|
||
| 6. Address feedback and update the PR. | ||
| * After pushing changes, add a comment to the PR mentioning the | ||
| reviewer(s) by name, stating the comments have been addressed. This is the best | ||
| way to ensure that the reviewer is notified that the code is ready to be reviewed | ||
| again. | ||
|
|
||
| 1. Go to step 7. | ||
|
|
||
| 1. Maintainer merges the pull request after final changes are accepted. Due to | ||
| tooling limitations, a Meta employee is required to merge the pull request. | ||
|
|
||
| ## Presto’s SQL Functions | ||
|
|
||
| Here are specific guidelines for contributing Presto SQL functions. | ||
| * As a PR author, please do not "Resolve Conversation" when review comments are | ||
| addressed. Instead, wait for the reviewer to verify the comment has been | ||
| addressed and resolve the conversation. | ||
|
|
||
| 7. Iterate on this process until your changes are reviewed and accepted by a | ||
| maintainer. At this point, a Meta employee will be required to merge your PR, | ||
| due to tooling limitations. | ||
|
|
||
| ## Coding Best Practices | ||
|
|
||
| When submitting code contributions to Velox, make sure to adhere to the | ||
| following best practices: | ||
|
|
||
| 1. **Coding Style**: Review and strictly follow our coding style document, | ||
| available in [`CODING_STYLE.md`](CODING_STYLE.md). | ||
| * Velox favors consistency over personal preference. If there are | ||
| technical reasons why a specific guideline should not be followed, | ||
| please start a separate discussion with the community to update the | ||
| coding style document first. | ||
| * If you are simply updating code that did not comply with the coding | ||
| style, please do so in a standalone PR isolated from other logic changes. | ||
|
|
||
| 2. **Small Incremental Changes**: If the change is large, work with the maintainers | ||
| on a plan to break and submit it as smaller (yet atomic) parts. | ||
| * [Research indicates](https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/) | ||
| that engineers can only effectively review up to | ||
| 400 lines of code at a time. The human brain can only process so much information | ||
| at a time; beyond that threshold the ability to find bugs and other flaws | ||
| decreases. | ||
| * As larger PRs usually take longer to review and iterate, they | ||
| tend to slow down the software development process. As much as possible, | ||
| split your work into smaller changes. | ||
|
|
||
| 3. **Unit tests**: With rare exceptions, every PR should contain unit tests | ||
| covering the logic added/modified. | ||
| * Unit tests protect our codebase from regressions, promote less coupled | ||
| APIs, and provide an executable form of documentation that’s useful for | ||
| new engineers reasoning about the codebase. | ||
| * Good unit tests are fast, isolated, repeatable, and exercise all APIs | ||
| including their edge cases. | ||
| * The lack of existing tests is not a good reason not to add tests to | ||
| your PR. If a component or API does not have a corresponding | ||
| unit test suite, please consider improving the codebase by first adding a | ||
| new unit test suite to ensure the existing behavior is correct. | ||
|
|
||
| 4. **Code Comments**: Appropriately add comments to your code and document APIs. | ||
| * As a library, Velox code is optimized for the reader, not the writer. | ||
| * Comments should capture information that was on the writer’s mind, but | ||
| could not be represented as code. The overall goal is to make the code more | ||
| obvious and remove obscurity. | ||
| * As a guideline, every file, class, member variable, and member function | ||
| that is not a getter/setter should be documented. | ||
| * As much as possible, try to avoid functions with very large bodies. In the | ||
| (rare) cases where large code blocks are needed, a good practice is to group | ||
| smaller blocks of related code, and precede them with a blank line and a | ||
| high-level comment explaining what the block does. | ||
|
|
||
| 5. **Benchmarks**: Add micro-benchmarks to support your claims. | ||
| * As needed, add micro-benchmark to objectively evaluate performance and | ||
| efficiency trade-offs. | ||
|
|
||
| 6. **APIs**: Carefully design APIs. | ||
| * As a library, Velox APIs should be intentional. External API should only | ||
| be deliberately created. | ||
| * As a rule of thumb, components should be deep and encapsulate as much | ||
| complexity as possible, and APIs should be narrow, minimizing dependencies | ||
| across components and preventing implementation details from leaking through | ||
| the API. | ||
|
|
||
| ## Adding Scalar Functions | ||
|
|
||
| Adding Presto and Spark functions are a good way to get yourself familiar with | ||
| Velox and the code review process. In addition to the general contribution | ||
| guidelines presented above, here are specific guidelines for contributing | ||
| functions: | ||
|
|
||
| 1. Read [How to add a scalar function?](https://facebookincubator.github.io/velox/develop/scalar-functions.html) guide. | ||
|
|
||
| 2. Use the following template for the PR title: Add xxx Presto function (replace xxx with the function name.) | ||
| 2. Use the following template for the PR title: Add xxx [Presto|Spark] function (replace xxx with the function name). | ||
| * Ensure the PR description contains a link to the function documentation | ||
| from Presto or Spark docs. | ||
| * Describe the function semantics and edge cases clearly. | ||
|
|
||
| 3. Add documentation for the new function to an .rst file under velox/docs/functions directory. | ||
| 3. Use Presto or Spark to check the function semantics. | ||
| * Try different edge cases to check whether the function returns null, or | ||
| throws, etc. | ||
| * Make sure to replicate the exact semantics. | ||
|
|
||
| 4. Functions in documentation are listed in alphabetical order. Make sure to | ||
| place the new function so that the order is preserved. | ||
| 4. Add tests exercising common inputs, all possible signatures and corner cases. | ||
| * Make sure the test cases are concise and easily readable. | ||
|
|
||
| 5. Use Presto to check the function semantics. Try different edge cases to see | ||
| whether the function returns null, or throws, or does something else. Make sure | ||
| to replicate Presto semantics exactly. | ||
| 5. Make sure that obvious inefficiencies are addressed. | ||
| * If appropriate, provide micro-benchmarks to support your claims with data. | ||
|
|
||
| 6. Add tests. | ||
| 4. Add documentation for the new function to an .rst file under velox/docs/functions directory. | ||
| * Functions in documentation are listed in alphabetical order. Make sure to | ||
| place the new function so that the order is preserved. | ||
|
|
||
| 7. Run [Fuzzer](https://facebookincubator.github.io/velox/develop/testing/fuzzer.html) | ||
| 6. Run [Fuzzer](https://facebookincubator.github.io/velox/develop/testing/fuzzer.html) | ||
| to test new the function in isolation as well as in combination with other functions. | ||
| Note: Fuzzer is under active development and the commands below may not work as is. | ||
| Consult the CircleCI configuration in .circleci/config.yml for the up-to-date command | ||
|
|
||