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 2021/03/21 09:00:12 UTC
[GitHub] [airflow] ankitdobhal opened a new pull request #14920: chore: Refactor code quality issues
ankitdobhal opened a new pull request #14920:
URL: https://github.com/apache/airflow/pull/14920
## Description
Hey π , I'm member of the Developer Outreach team at DeepSource and ran DeepSource analysis on my fork of the repo. It found some interesting [code quality improvements](https://deepsource.io/gh/ankitdobhal/airflow/issues) to consider.
This PR fixes a few of the issues detected for you to assess if it is right for you.
Happy to provide the tweaks separately otherwise :)
## Important changes
- Remove duplicate elements during set declaration
- Remove redundant 'None' default
- Remove reimported module
- Use shallow copy of dictionary for iteration
- Added .deepsource.toml file for continuous analysis on bug risks/performance/code-quality issues on new changes.
--
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
[GitHub] [airflow] turbaszek commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598260556
##########
File path: airflow/models/dag.py
##########
@@ -2335,8 +2334,6 @@ def factory(*args, **kwargs):
STATICA_HACK = True
globals()['kcah_acitats'[::-1].upper()] = False
if STATICA_HACK: # pragma: no cover
- # Let pylint know about these relationships, without introducing an import cycle
- from sqlalchemy.orm import relationship
Review comment:
Oh, indeed. We import this at the beginning π
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#issuecomment-803536409
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itβs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better π.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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
[GitHub] [airflow] ankitdobhal commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
ankitdobhal commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r599284089
##########
File path: airflow/sensors/smart_sensor.py
##########
@@ -672,13 +672,13 @@ def _execute_sensor_work(self, sensor_work):
def flush_cached_sensor_poke_results(self):
"""Flush outdated cached sensor states saved in previous loop."""
- for key, cached_work in self.cached_dedup_works.items():
+ for key, cached_work in self.cached_dedup_works.copy().items():
Review comment:
Dictionaries are represented by a hash table and adding or removing items while iterating over it will alter the iteration order. This will cause a RuntimeError.
If you need to add items to the dictionary during iteration, it is recommended to iterate over a shallow `copy` of the dictionary.
You can checkout brief here : https://deepsource.io/gh/ankitdobhal/airflow/issue/PTC-W0056/description
--
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
[GitHub] [airflow] turbaszek commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598260699
##########
File path: airflow/models/dag.py
##########
@@ -2029,7 +2029,6 @@ def get_serialized_fields(cls):
'user_defined_filters',
'user_defined_macros',
'partial',
- '_old_context_manager_dags',
Review comment:
@kaxil can you take a look?
--
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
[GitHub] [airflow] turbaszek commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598260556
##########
File path: airflow/models/dag.py
##########
@@ -2335,8 +2334,6 @@ def factory(*args, **kwargs):
STATICA_HACK = True
globals()['kcah_acitats'[::-1].upper()] = False
if STATICA_HACK: # pragma: no cover
- # Let pylint know about these relationships, without introducing an import cycle
- from sqlalchemy.orm import relationship
Review comment:
Can you explain this?
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598947885
##########
File path: airflow/models/dag.py
##########
@@ -2029,7 +2029,6 @@ def get_serialized_fields(cls):
'user_defined_filters',
'user_defined_macros',
'partial',
- '_old_context_manager_dags',
Review comment:
Yeah it is safe, was just a duplicate from L2025
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598949266
##########
File path: airflow/sensors/smart_sensor.py
##########
@@ -672,13 +672,13 @@ def _execute_sensor_work(self, sensor_work):
def flush_cached_sensor_poke_results(self):
"""Flush outdated cached sensor states saved in previous loop."""
- for key, cached_work in self.cached_dedup_works.items():
+ for key, cached_work in self.cached_dedup_works.copy().items():
Review comment:
hmm Why do we need `copy()`?
--
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
[GitHub] [airflow] ankitdobhal commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
ankitdobhal commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r599285236
##########
File path: .deepsource.toml
##########
@@ -0,0 +1,25 @@
+version = 1
+
+test_patterns = ["tests/**"]
+
+[[analyzers]]
+name = "python"
+enabled = true
+
+ [analyzers.meta]
+ runtime_version = "3.x.x"
+
+[[analyzers]]
+name = "docker"
+enabled = true
+
+ [analyzers.meta]
+ dockerfile_paths = ["Dockerfile"]
+
+[[analyzers]]
+name = "javascript"
+enabled = true
+
+ [analyzers.meta]
+ environment = ["browser"]
+
Review comment:
@kaxil have removed `.deepsource.toml` file.
Thanks
##########
File path: .deepsource.toml
##########
@@ -0,0 +1,25 @@
+version = 1
+
+test_patterns = ["tests/**"]
+
+[[analyzers]]
+name = "python"
+enabled = true
+
+ [analyzers.meta]
+ runtime_version = "3.x.x"
+
+[[analyzers]]
+name = "docker"
+enabled = true
+
+ [analyzers.meta]
+ dockerfile_paths = ["Dockerfile"]
+
+[[analyzers]]
+name = "javascript"
+enabled = true
+
+ [analyzers.meta]
+ environment = ["browser"]
+
Review comment:
@kaxil removed `.deepsource.toml` file.
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
[GitHub] [airflow] kaxil commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598947280
##########
File path: .deepsource.toml
##########
@@ -0,0 +1,25 @@
+version = 1
+
+test_patterns = ["tests/**"]
+
+[[analyzers]]
+name = "python"
+enabled = true
+
+ [analyzers.meta]
+ runtime_version = "3.x.x"
+
+[[analyzers]]
+name = "docker"
+enabled = true
+
+ [analyzers.meta]
+ dockerfile_paths = ["Dockerfile"]
+
+[[analyzers]]
+name = "javascript"
+enabled = true
+
+ [analyzers.meta]
+ environment = ["browser"]
+
Review comment:
Let's remove this please -- we don't want to add yet another scanner
##########
File path: .deepsource.toml
##########
@@ -0,0 +1,25 @@
+version = 1
+
+test_patterns = ["tests/**"]
+
+[[analyzers]]
+name = "python"
+enabled = true
+
+ [analyzers.meta]
+ runtime_version = "3.x.x"
+
+[[analyzers]]
+name = "docker"
+enabled = true
+
+ [analyzers.meta]
+ dockerfile_paths = ["Dockerfile"]
+
+[[analyzers]]
+name = "javascript"
+enabled = true
+
+ [analyzers.meta]
+ environment = ["browser"]
+
Review comment:
Let's remove this file please -- we don't want to add yet another scanner
--
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
[GitHub] [airflow] kaxil commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r598948184
##########
File path: airflow/models/dag.py
##########
@@ -2335,8 +2334,6 @@ def factory(*args, **kwargs):
STATICA_HACK = True
globals()['kcah_acitats'[::-1].upper()] = False
if STATICA_HACK: # pragma: no cover
- # Let pylint know about these relationships, without introducing an import cycle
- from sqlalchemy.orm import relationship
Review comment:
cc @ashb
--
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
[GitHub] [airflow] ankitdobhal commented on a change in pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
ankitdobhal commented on a change in pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#discussion_r599284089
##########
File path: airflow/sensors/smart_sensor.py
##########
@@ -672,13 +672,13 @@ def _execute_sensor_work(self, sensor_work):
def flush_cached_sensor_poke_results(self):
"""Flush outdated cached sensor states saved in previous loop."""
- for key, cached_work in self.cached_dedup_works.items():
+ for key, cached_work in self.cached_dedup_works.copy().items():
Review comment:
Dictionaries are represented by a hash table and adding or removing items while iterating over it will alter the iteration order. This will cause a RuntimeError.
If you need to add items to the dictionary during iteration, it is recommended to iterate over a shallow `copy` of the dictionary.
--
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
[GitHub] [airflow] kaxil merged pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #14920:
URL: https://github.com/apache/airflow/pull/14920
--
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
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #14920: chore: Refactor code quality issues
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #14920:
URL: https://github.com/apache/airflow/pull/14920#issuecomment-804779582
Awesome work, congrats on your first merged pull request!
--
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