You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/02/21 12:33:37 UTC

[GitHub] [airflow] ashb opened a new pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

ashb opened a new pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483
 
 
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x] Description above provides context of the change
   - [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [x] Unit tests coverage for changes (not needed for documentation changes)
   - [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x] Relevant documentation is updated including usage instructions.
   - [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382563517
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -321,6 +326,47 @@ Your code must pass all the static code checks in Travis CI in order to be eligi
 The easiest way to make sure your code is good before pushing is to use pre-commit checks locally
 as described in the static code checks documentation.
 
+.. _coding_style:
+
+Coding style and best practices
+===============================
+
+Most of your coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
+on every pull request), but there are some rules that are not yet automated and are more Airflow specific or
+semantic than style
+
+Database Session Handling
+-------------------------
+
+**Explicit is better than implicit.** If a function accepts a ``session`` parameter it should not commit the
+transaction itself. Session management is up to the caller.
+
+To make this easier there is the ``create_session`` helper:
+
+.. code-block:: python
+
+    from airflow.utils.db import create_session
+
+    def my_call(*args, session):
+      ...
+      # You MUST not commit the session here.
+
+    with create_session() as session:
+        my_call(*args, session=session)
+
+If this function is designed to be called by "end-users" (i.e. DAG authors) then using the ``@provide_session`` wrapper is okay:
+
+.. code-block:: python
+
+    from airflow.utils.db import provide_session
 
 Review comment:
   ```suggestion
       from airflow.utils.session import provide_session
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382563680
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -321,6 +326,47 @@ Your code must pass all the static code checks in Travis CI in order to be eligi
 The easiest way to make sure your code is good before pushing is to use pre-commit checks locally
 as described in the static code checks documentation.
 
+.. _coding_style:
+
+Coding style and best practices
+===============================
+
+Most of your coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
 
 Review comment:
   ```suggestion
   Most of our coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382558055
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -142,6 +142,11 @@ these guidelines:
     It will help you make sure you do not break the build with your PR and
     that you help increase coverage.
 
+-   Follow our project's `Coding style and best practices`_.
+
+    These are things that aren't currently enforced programtically (either because they are two hard or just
 
 Review comment:
   ```suggestion
       These are things that aren't currently enforced programtically (either because they are too hard or just
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589651165
 
 
   Good idea Tomek, will open a PR there too

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb edited a comment on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb edited a comment on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589651165
 
 
   Good idea Tomek, will open a PR there too. It's in here isn't it :)
   
   Updated.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil merged pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] nuclearpinguin commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589649536
 
 
   @ash should we link those best practices in `firstPRWelcomeComment` in boring cyborg?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] yuqian90 commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
yuqian90 commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589648022
 
 
   Lgtm thanks!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589635198
 
 
   /cc @yuqian90 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#issuecomment-589646566
 
 
   Lol thanks Kaxil. Rushed PRs are never worth it :D 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382563069
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -321,6 +326,47 @@ Your code must pass all the static code checks in Travis CI in order to be eligi
 The easiest way to make sure your code is good before pushing is to use pre-commit checks locally
 as described in the static code checks documentation.
 
+.. _coding_style:
+
+Coding style and best practices
+===============================
+
+Most of your coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
+on every pull request), but there are some rules that are not yet automated and are more Airflow specific or
+semantic than style
+
+Database Session Handling
+-------------------------
+
+**Explicit is better than implicit.** If a function accepts a ``session`` parameter it should not commit the
+transaction itself. Session management is up to the caller.
+
+To make this easier there is the ``create_session`` helper:
+
+.. code-block:: python
+
+    from airflow.utils.db import create_session
 
 Review comment:
   ```suggestion
       from airflow.utils.session import create_session
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382563376
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -321,6 +326,47 @@ Your code must pass all the static code checks in Travis CI in order to be eligi
 The easiest way to make sure your code is good before pushing is to use pre-commit checks locally
 as described in the static code checks documentation.
 
+.. _coding_style:
+
+Coding style and best practices
+===============================
+
+Most of your coding style rules are enforced programmatically by flake8 and pylint (which are run automatically
+on every pull request), but there are some rules that are not yet automated and are more Airflow specific or
+semantic than style
+
+Database Session Handling
+-------------------------
+
+**Explicit is better than implicit.** If a function accepts a ``session`` parameter it should not commit the
+transaction itself. Session management is up to the caller.
+
+To make this easier there is the ``create_session`` helper:
+
+.. code-block:: python
+
+    from airflow.utils.db import create_session
+
+    def my_call(*args, session):
+      ...
+      # You MUST not commit the session here.
+
+    with create_session() as session:
+        my_call(*args, session=session)
+
+If this function is designed to be called by "end-users" (i.e. DAG authors) then using the ``@provide_session`` wrapper is okay:
+
+.. code-block:: python
+
+    from airflow.utils.db import provide_session
+
+    ...
+
+    @session
 
 Review comment:
   ```suggestion
       @provide_session
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] ashb commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #7483: [AIRFLOW-XXXX] Document new session handling guidelines
URL: https://github.com/apache/airflow/pull/7483#discussion_r382557976
 
 

 ##########
 File path: CONTRIBUTING.rst
 ##########
 @@ -142,6 +142,11 @@ these guidelines:
     It will help you make sure you do not break the build with your PR and
     that you help increase coverage.
 
+-   Follow our project's `Coding style and best practices`_.
+
+    These are things that aren't currently enforced programtically (either because they are two hard or just
 
 Review comment:
   Bah.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services