potiuk commented on code in PR #36472:
URL: https://github.com/apache/airflow/pull/36472#discussion_r1437833370
##########
CONTRIBUTING.rst:
##########
@@ -475,38 +586,59 @@ Pull Request Guidelines
Before you submit a pull request (PR) from your forked repo, check that it
meets
these guidelines:
-- Include tests, either as doctests, unit tests, or both, to your pull
- request.
+- Include tests, either as doctests, unit tests, or both, to your pull
request.
The airflow repo uses `GitHub Actions
<https://help.github.com/en/actions>`__ to
run the tests and `codecov <https://codecov.io/gh/apache/airflow>`__ to
track
coverage. You can set up both for free on your fork. It will help you make
sure you do not
break the build with your PR and that you help increase coverage.
+ Also we advise to install locally `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__ to
+ apply various checks, code generation and formatting locally at the time
you make a local commit - which
+ gives you near-immediate feedback on things you need to fix before you
push your code to the PR, or in
+ many case it will even fix it for you locally so that you can add and
commit it straight away.
-- Follow our project's `Coding style and best practices`_.
-
- These are things that aren't currently enforced programmatically (either
because they are too hard or just
- not yet done.)
+- Follow our project's `Coding style and best practices`_. Usually we
attempt to enforce the practices by
+ having appropriate pre-commits These are things amongst them that aren't
currently enforced
+ programmatically (either because they are too hard or just not yet done).
-- `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__, and
resolve all conflicts.
+- We prefer that you ``rebase`` your Pull Request (and do it quite often)
rather than merge. It leads to
+ easier reviews and cleaner changes where you know exactly what changes
you've done. You can learn more
+ about rebase vs. merge workflow in `Rebase and merge your pull request
<https://github.blog/2016-09-26-rebase-and-merge-pull-requests/>`__
+ and `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__.
Make sure to resolve all conflicts
+ during rebase.
-- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one commit, regardless of the number of commits in
your PR. During the review cycle, you can keep a commit history for easier
review, but if you need to, you can also squash all commits to reduce the
maintenance burden during rebase.
+- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one
+ commit, regardless of the number of commits in your PR. During the review
cycle, you can keep a commit
+ history for easier review, but if you need to, you can also squash all
commits to reduce the
+ maintenance burden during rebase.
-- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header
- to all new files.
+- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header to all new files (if you
+ have ``pre-commit`` installed, pre-commit will do it automatically for
you. This is one of the reasons
+ why we recommend using ``pre-commit``.
- If you have `pre-commit hooks <STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
enabled, they automatically add
- license headers during commit.
-
-- If your pull request adds functionality, make sure to update the docs as
part
- of the same PR. Doc string is often sufficient. Make sure to follow the
- Sphinx compatible standards.
+- If your pull request adds functionality, make sure to update the docs as
part of the same PR, not only
+ code and tests. Doc string is often sufficient. Make sure to follow the
Sphinx compatible standards.
- Make sure your code fulfills all the
`static code checks <STATIC_CODE_CHECKS.rst#static-code-checks>`__ we have
in our code. The easiest way
- to make sure of that is to use `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
-
-- Run tests locally before opening PR.
+ to make sure of that is - again - to install `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
+
+- Make your PR small and focuses on one change only - avoid adding unrelated
changes, mixing adding features
Review Comment:
yep
##########
CONTRIBUTING.rst:
##########
@@ -475,38 +586,59 @@ Pull Request Guidelines
Before you submit a pull request (PR) from your forked repo, check that it
meets
these guidelines:
-- Include tests, either as doctests, unit tests, or both, to your pull
- request.
+- Include tests, either as doctests, unit tests, or both, to your pull
request.
The airflow repo uses `GitHub Actions
<https://help.github.com/en/actions>`__ to
run the tests and `codecov <https://codecov.io/gh/apache/airflow>`__ to
track
coverage. You can set up both for free on your fork. It will help you make
sure you do not
break the build with your PR and that you help increase coverage.
+ Also we advise to install locally `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__ to
+ apply various checks, code generation and formatting locally at the time
you make a local commit - which
+ gives you near-immediate feedback on things you need to fix before you
push your code to the PR, or in
+ many case it will even fix it for you locally so that you can add and
commit it straight away.
-- Follow our project's `Coding style and best practices`_.
-
- These are things that aren't currently enforced programmatically (either
because they are too hard or just
- not yet done.)
+- Follow our project's `Coding style and best practices`_. Usually we
attempt to enforce the practices by
+ having appropriate pre-commits These are things amongst them that aren't
currently enforced
+ programmatically (either because they are too hard or just not yet done).
-- `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__, and
resolve all conflicts.
+- We prefer that you ``rebase`` your Pull Request (and do it quite often)
rather than merge. It leads to
+ easier reviews and cleaner changes where you know exactly what changes
you've done. You can learn more
+ about rebase vs. merge workflow in `Rebase and merge your pull request
<https://github.blog/2016-09-26-rebase-and-merge-pull-requests/>`__
+ and `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__.
Make sure to resolve all conflicts
+ during rebase.
-- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one commit, regardless of the number of commits in
your PR. During the review cycle, you can keep a commit history for easier
review, but if you need to, you can also squash all commits to reduce the
maintenance burden during rebase.
+- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one
+ commit, regardless of the number of commits in your PR. During the review
cycle, you can keep a commit
+ history for easier review, but if you need to, you can also squash all
commits to reduce the
+ maintenance burden during rebase.
-- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header
- to all new files.
+- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header to all new files (if you
+ have ``pre-commit`` installed, pre-commit will do it automatically for
you. This is one of the reasons
+ why we recommend using ``pre-commit``.
- If you have `pre-commit hooks <STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
enabled, they automatically add
- license headers during commit.
-
-- If your pull request adds functionality, make sure to update the docs as
part
- of the same PR. Doc string is often sufficient. Make sure to follow the
- Sphinx compatible standards.
+- If your pull request adds functionality, make sure to update the docs as
part of the same PR, not only
+ code and tests. Doc string is often sufficient. Make sure to follow the
Sphinx compatible standards.
- Make sure your code fulfills all the
`static code checks <STATIC_CODE_CHECKS.rst#static-code-checks>`__ we have
in our code. The easiest way
- to make sure of that is to use `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
-
-- Run tests locally before opening PR.
+ to make sure of that is - again - to install `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
+
+- Make your PR small and focuses on one change only - avoid adding unrelated
changes, mixing adding features
+ and refactoring. Keeping to that rule will make it easier to review your
PR and will make it easier to
Review Comment:
yep
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
