pankajkoti commented on code in PR #36472:
URL: https://github.com/apache/airflow/pull/36472#discussion_r1437734498
##########
CONTRIBUTING.rst:
##########
@@ -464,9 +487,97 @@ Step 5: Pass PR Review
Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
when merging PRs and your commit will be squashed to single commit.
-You need to have review of at least one committer (if you are committer
yourself, it has to be
+When the maintainer starts conversations it is expected that you respond to
questions, suggestions, doubts,
+and generally it's great if all such conversations seem to converge to a
common understanding. You do not
+necessarily have to apply all the suggestions (often they are just opinions
and suggestions even if they are
+coming from seasoned maintainers) - it's perfectly ok that you respond to it
with your own opinions and
+understanding of the problem and your approach and if you have good arguments,
presenting them is a good idea.
+
+The reviewers might leave several types of responses:
+
+* ``General PR comment`` - which usually means that there is a
question/opinion/suggestion on how the PR can be
+ improved, or it's an ask to explain how you understand the PR. You can
usually quote some parts of such
+ general comment and respond to it in your comments. Often comments that are
raising questions in general
+ might lead to different discussions, even a request to move the discussion
to the devlist or even lead to
+ completely new PRs created as a spin-off of the discussion.
+
+* ``Comment/Conversation around specific lines of code`` - such conversation
usually flags a potential
+ improvement, or a potential problem with the code. It's a good idea to
respond to such comments and explain
+ your approach and understanding of the problem. If you do not agree with the
comment, you can explain why
+ you think your approach is better. If you agree with the comment, you can
explain why you did not do it
Review Comment:
```suggestion
you think your approach is better. If you agree with the comment, you can
optionally explain why you did not do it
```
Can we make this optional, please? IMO, I would generally not have thought
of the approach in the first place or I would have been naive/unaware/beginner.
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
+ to get reviewed before it reaches ``Ready for review`` status. This might
happen if you want to get initial
+ feedback on the direction of your PR or if you want to get feedback on the
design of your PR.
+
+6. Avoid @-mentioning individual committers in your PR, unless you have good
reason to believe that they are
Review Comment:
A general question on the usage of the word `committers` throughout here. Do
we think the word `maintainers` could apply here more than `committers`?
##########
CONTRIBUTING.rst:
##########
@@ -464,9 +487,97 @@ Step 5: Pass PR Review
Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
when merging PRs and your commit will be squashed to single commit.
-You need to have review of at least one committer (if you are committer
yourself, it has to be
+When the maintainer starts conversations it is expected that you respond to
questions, suggestions, doubts,
+and generally it's great if all such conversations seem to converge to a
common understanding. You do not
+necessarily have to apply all the suggestions (often they are just opinions
and suggestions even if they are
+coming from seasoned maintainers) - it's perfectly ok that you respond to it
with your own opinions and
+understanding of the problem and your approach and if you have good arguments,
presenting them is a good idea.
+
+The reviewers might leave several types of responses:
+
+* ``General PR comment`` - which usually means that there is a
question/opinion/suggestion on how the PR can be
+ improved, or it's an ask to explain how you understand the PR. You can
usually quote some parts of such
+ general comment and respond to it in your comments. Often comments that are
raising questions in general
+ might lead to different discussions, even a request to move the discussion
to the devlist or even lead to
+ completely new PRs created as a spin-off of the discussion.
+
+* ``Comment/Conversation around specific lines of code`` - such conversation
usually flags a potential
+ improvement, or a potential problem with the code. It's a good idea to
respond to such comments and explain
+ your approach and understanding of the problem. If you do not agree with the
comment, you can explain why
+ you think your approach is better. If you agree with the comment, you can
explain why you did not do it
+ this way in the first place. If you agree with the comment and you think
it's a good idea, you can just
+ apply the suggestion and push the change to your PR. You can also resolve
the conversation if you think the
+ problem raised in the comment or ask the reviewer to re-review, confirm it
and resolve the conversation if
+ you are not sure. If you do not understand the comment, you can ask for
clarifications. Generally assume
+ good intention of the person who is reviewing your code, they are not
criticising you, at most they care
+ about the quality of the code and the project and want to make sure that the
code is as good as possible.
+
+ It's ok to mark the conversation resolved by anyone who can do it - it could
be the author, that thinks
+ the arguments are changes implemented make the conversation resolved, or the
maintainer/person who
+ started the conversation or it can be even marked as resolved by the
maintainer who attempts to merge the
+ PR and thinks that all conversations are resolved. However if you want to
make sure attention and decision
+ on merging the PR is given by maintainer, make sure you monitor, follow-up
and close the conversations when
+ you think they are resolved (ideally explaining why you think the
conversation is resolved).
+
+* ``Request changes`` - this is where maintainer is pretty sure that you
should make a change to your PR
+ because it contains serious flaw, design misconception, or a bug or it is
just not in-line with the common
+ approach Airflow community took on the issue. Usually you should respond to
such request and either fix
+ the problem or convince the maintainer that they were wrong (it happens more
often than you think).
+ Sometimes even if you do not agree with the request, it's a good idea to
make the change anyway, because
+ it might be a good idea to follow the common approach in the project.
Sometimes it might even happen that
+ two maintainer will have completely different opinions on the same issue and
you will have to lead the
+ discussion to try to achieve consensus. If you cannot achieve consensus and
you think it's an important
+ issue, you can ask for a vote on the issue by raising a devlist discussion -
where you explain your case
+ and follow up the discussion with a vote when you cannot achieve consensus
there. The ``Request changes``
+ status can be withdrawn by the maintainer, but if they don't - such PR
cannot be merged - maintainers have
+ the right to veto any code modification according to the `Apache Software
Foundation rules
<https://www.apache.org/foundation/voting.html#votes-on-code-modification>`_.
+
+* ``Approval`` - this is given by a maintainer after the code has been
reviewed and the maintainer agrees that
+ it is a good idea to merge it. There might still be some unresolved
conversations, requests and questions on
+ such PR and you are expected to resolve them before the PR is merged. But
the ``Approval`` status is a sign
+ of trust from the maintainer who gave the approval that they think the PR is
good enough as long as their
+ comments will be resolved and they put the trust in the hands of the author
and - possibly - other
+ maintainers who will merge the request that they can do that without
follow-up re-review and verification.
+
+
+You need to have ``Approval`` of at least one committer (if you are committer
yourself, it has to be
another committer). Ideally you should have 2 or more committers reviewing the
code that touches
-the core of Airflow.
+the core of Airflow - we do not have enforcement about ``2+`` reviewers
required for Core of Airflow,
+but Maintainers will generally ask in the PR if they think second review is
needed.
Review Comment:
```suggestion
but maintainers will generally ask in the PR if they think second review is
needed.
```
##########
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
Review Comment:
```suggestion
having appropriate pre-commits. These are things amongst them that
aren't currently enforced
```
##########
CONTRIBUTING.rst:
##########
@@ -464,9 +487,97 @@ Step 5: Pass PR Review
Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
when merging PRs and your commit will be squashed to single commit.
-You need to have review of at least one committer (if you are committer
yourself, it has to be
+When the maintainer starts conversations it is expected that you respond to
questions, suggestions, doubts,
+and generally it's great if all such conversations seem to converge to a
common understanding. You do not
+necessarily have to apply all the suggestions (often they are just opinions
and suggestions even if they are
+coming from seasoned maintainers) - it's perfectly ok that you respond to it
with your own opinions and
+understanding of the problem and your approach and if you have good arguments,
presenting them is a good idea.
+
+The reviewers might leave several types of responses:
+
+* ``General PR comment`` - which usually means that there is a
question/opinion/suggestion on how the PR can be
+ improved, or it's an ask to explain how you understand the PR. You can
usually quote some parts of such
+ general comment and respond to it in your comments. Often comments that are
raising questions in general
+ might lead to different discussions, even a request to move the discussion
to the devlist or even lead to
+ completely new PRs created as a spin-off of the discussion.
+
+* ``Comment/Conversation around specific lines of code`` - such conversation
usually flags a potential
+ improvement, or a potential problem with the code. It's a good idea to
respond to such comments and explain
+ your approach and understanding of the problem. If you do not agree with the
comment, you can explain why
+ you think your approach is better. If you agree with the comment, you can
explain why you did not do it
+ this way in the first place. If you agree with the comment and you think
it's a good idea, you can just
+ apply the suggestion and push the change to your PR. You can also resolve
the conversation if you think the
+ problem raised in the comment or ask the reviewer to re-review, confirm it
and resolve the conversation if
+ you are not sure. If you do not understand the comment, you can ask for
clarifications. Generally assume
+ good intention of the person who is reviewing your code, they are not
criticising you, at most they care
+ about the quality of the code and the project and want to make sure that the
code is as good as possible.
+
+ It's ok to mark the conversation resolved by anyone who can do it - it could
be the author, that thinks
+ the arguments are changes implemented make the conversation resolved, or the
maintainer/person who
+ started the conversation or it can be even marked as resolved by the
maintainer who attempts to merge the
+ PR and thinks that all conversations are resolved. However if you want to
make sure attention and decision
+ on merging the PR is given by maintainer, make sure you monitor, follow-up
and close the conversations when
+ you think they are resolved (ideally explaining why you think the
conversation is resolved).
+
+* ``Request changes`` - this is where maintainer is pretty sure that you
should make a change to your PR
+ because it contains serious flaw, design misconception, or a bug or it is
just not in-line with the common
+ approach Airflow community took on the issue. Usually you should respond to
such request and either fix
+ the problem or convince the maintainer that they were wrong (it happens more
often than you think).
+ Sometimes even if you do not agree with the request, it's a good idea to
make the change anyway, because
+ it might be a good idea to follow the common approach in the project.
Sometimes it might even happen that
+ two maintainer will have completely different opinions on the same issue and
you will have to lead the
Review Comment:
```suggestion
two maintainers will have completely different opinions on the same issue
and you will have to lead the
```
##########
CONTRIBUTING.rst:
##########
@@ -464,9 +487,97 @@ Step 5: Pass PR Review
Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
when merging PRs and your commit will be squashed to single commit.
-You need to have review of at least one committer (if you are committer
yourself, it has to be
+When the maintainer starts conversations it is expected that you respond to
questions, suggestions, doubts,
+and generally it's great if all such conversations seem to converge to a
common understanding. You do not
+necessarily have to apply all the suggestions (often they are just opinions
and suggestions even if they are
+coming from seasoned maintainers) - it's perfectly ok that you respond to it
with your own opinions and
+understanding of the problem and your approach and if you have good arguments,
presenting them is a good idea.
+
+The reviewers might leave several types of responses:
+
+* ``General PR comment`` - which usually means that there is a
question/opinion/suggestion on how the PR can be
+ improved, or it's an ask to explain how you understand the PR. You can
usually quote some parts of such
+ general comment and respond to it in your comments. Often comments that are
raising questions in general
+ might lead to different discussions, even a request to move the discussion
to the devlist or even lead to
+ completely new PRs created as a spin-off of the discussion.
+
+* ``Comment/Conversation around specific lines of code`` - such conversation
usually flags a potential
+ improvement, or a potential problem with the code. It's a good idea to
respond to such comments and explain
+ your approach and understanding of the problem. If you do not agree with the
comment, you can explain why
+ you think your approach is better. If you agree with the comment, you can
explain why you did not do it
+ this way in the first place. If you agree with the comment and you think
it's a good idea, you can just
+ apply the suggestion and push the change to your PR. You can also resolve
the conversation if you think the
+ problem raised in the comment or ask the reviewer to re-review, confirm it
and resolve the conversation if
+ you are not sure. If you do not understand the comment, you can ask for
clarifications. Generally assume
+ good intention of the person who is reviewing your code, they are not
criticising you, at most they care
+ about the quality of the code and the project and want to make sure that the
code is as good as possible.
+
+ It's ok to mark the conversation resolved by anyone who can do it - it could
be the author, that thinks
+ the arguments are changes implemented make the conversation resolved, or the
maintainer/person who
+ started the conversation or it can be even marked as resolved by the
maintainer who attempts to merge the
+ PR and thinks that all conversations are resolved. However if you want to
make sure attention and decision
+ on merging the PR is given by maintainer, make sure you monitor, follow-up
and close the conversations when
+ you think they are resolved (ideally explaining why you think the
conversation is resolved).
+
+* ``Request changes`` - this is where maintainer is pretty sure that you
should make a change to your PR
+ because it contains serious flaw, design misconception, or a bug or it is
just not in-line with the common
+ approach Airflow community took on the issue. Usually you should respond to
such request and either fix
+ the problem or convince the maintainer that they were wrong (it happens more
often than you think).
+ Sometimes even if you do not agree with the request, it's a good idea to
make the change anyway, because
+ it might be a good idea to follow the common approach in the project.
Sometimes it might even happen that
+ two maintainer will have completely different opinions on the same issue and
you will have to lead the
+ discussion to try to achieve consensus. If you cannot achieve consensus and
you think it's an important
+ issue, you can ask for a vote on the issue by raising a devlist discussion -
where you explain your case
+ and follow up the discussion with a vote when you cannot achieve consensus
there. The ``Request changes``
+ status can be withdrawn by the maintainer, but if they don't - such PR
cannot be merged - maintainers have
+ the right to veto any code modification according to the `Apache Software
Foundation rules
<https://www.apache.org/foundation/voting.html#votes-on-code-modification>`_.
+
+* ``Approval`` - this is given by a maintainer after the code has been
reviewed and the maintainer agrees that
+ it is a good idea to merge it. There might still be some unresolved
conversations, requests and questions on
+ such PR and you are expected to resolve them before the PR is merged. But
the ``Approval`` status is a sign
+ of trust from the maintainer who gave the approval that they think the PR is
good enough as long as their
+ comments will be resolved and they put the trust in the hands of the author
and - possibly - other
+ maintainers who will merge the request that they can do that without
follow-up re-review and verification.
+
+
+You need to have ``Approval`` of at least one committer (if you are committer
yourself, it has to be
another committer). Ideally you should have 2 or more committers reviewing the
code that touches
-the core of Airflow.
+the core of Airflow - we do not have enforcement about ``2+`` reviewers
required for Core of Airflow,
+but Maintainers will generally ask in the PR if they think second review is
needed.
+
+Your PR can be merged by the reviewer or another maintainer who will see that
the
Review Comment:
```suggestion
Your PR can be merged by a maintainer who will see that the
```
##########
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:
```suggestion
and refactoring. Keeping to that rule will make it easier to review your
PR and will make it easier
```
##########
CONTRIBUTING.rst:
##########
@@ -464,9 +487,97 @@ Step 5: Pass PR Review
Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
when merging PRs and your commit will be squashed to single commit.
-You need to have review of at least one committer (if you are committer
yourself, it has to be
+When the maintainer starts conversations it is expected that you respond to
questions, suggestions, doubts,
Review Comment:
```suggestion
When a reviewer starts a conversation it is expected that you respond to
questions, suggestions, doubts,
```
I think the check for the necessity of resolved conversations would apply to
all conversations irrespective of it being from a maintainer or not, right?
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
+ to get reviewed before it reaches ``Ready for review`` status. This might
happen if you want to get initial
+ feedback on the direction of your PR or if you want to get feedback on the
design of your PR.
+
+6. Avoid @-mentioning individual committers in your PR, unless you have good
reason to believe that they are
+ available, have time and interest in your PR. Generally speaking there are
no "exclusive" reviewers for
+ different parts of the code and reviewers review PRs and respond when they
have some free time to spare and
+ when they feel they can provide valuable feedback. If you want to get
attention of committers, you can just
+ follow-up on your PR and ask for review in general, however be considerate
and do not expect "immediate"
+ reviews. People review when they have time, most of the people who are
committers do such reviews in their
+ free time, which is taken away from their families and other interests so
allow sufficient time before you
+ follow-up - but If you see no reaction in several days, do follow-up, as
with the number of PRs we have
Review Comment:
```suggestion
follow-up - but if you see no reaction in several days, do follow-up, as
with the number of PRs we have
```
##########
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
Review Comment:
```suggestion
apply various checks, code generation and formatting at the time you
make a local commit - which
```
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
+ to get reviewed before it reaches ``Ready for review`` status. This might
happen if you want to get initial
+ feedback on the direction of your PR or if you want to get feedback on the
design of your PR.
+
+6. Avoid @-mentioning individual committers in your PR, unless you have good
reason to believe that they are
+ available, have time and interest in your PR. Generally speaking there are
no "exclusive" reviewers for
+ different parts of the code and reviewers review PRs and respond when they
have some free time to spare and
Review Comment:
```suggestion
different parts of the code. Reviewers review PRs and respond when they
have some free time to spare and
```
##########
.asf.yaml:
##########
@@ -44,37 +44,47 @@ github:
main:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
+ required_conversation_resolution: true
v1-10-stable:
required_pull_request_reviews:
required_approving_review_count: 1
v2-0-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-1-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-2-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-3-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-4-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-5-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-6-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-7-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-8-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
-
+ required_approving_review_count: 1
+ required_linear_history: true
Review Comment:
How does this work for existing branches which may not have linear history?
Also, for newer vX-X-stable branches is it always possible to have linear
histories during the release management activities? Asking just our of
curiosity to understand this one.
##########
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
+ for release managers if they decide that your change should be
cherry-picked to release it in a bug-fix
+ release of Airflow. If you want to add a new feature and refactor the
code, it's better to split the
+ PR to several smaller PRs. It's also quite a good and common idea to keep
a big ``Draft`` PR if you have
+ a bigger change that you want to make and then create smaller PRs from it
that are easier to review and
+ merge and cherry-pick. It take a long time (and a lot of attention and
focus of a reviewer to review
Review Comment:
```suggestion
merge and cherry-pick. It takes a long time (and a lot of attention and
focus) of a reviewer to review
```
##########
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:
```suggestion
- Make your PR small and focused on one change only - avoid adding
unrelated changes, mixing adding features
```
##########
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``.
Review Comment:
```suggestion
why we recommend using ``pre-commit``).
```
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
