[AIRFLOW-2867] Refactor code to conform Python standards & guidelines #3714

kaxil · 2018-08-07T20:17:22Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-2867

Description

Here are some details about my PR, including screenshots of any UI changes:
Dictionary creation should be written by dictionary literal
Python’s default arguments are evaluated once when the function is defined, not each time the function is called (like it is in say, Ruby). This means that if you use a mutable default argument and mutate it, you will and have mutated that object for all future calls to the function as well.
Functions calling sets which can be replaced by set literal are now replaced by set literal
Replace list literals
Some of the static methods haven't been set static
Remove redundant parentheses

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
N/a, Nothing new added

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes git diff upstream/master -u -- "*.py" | flake8 --diff

ashb · 2018-08-07T20:18:18Z

airflow/configuration.py

@@ -186,7 +186,8 @@ def _validate(self):

        self.is_validated = True

-    def _get_env_var_option(self, section, key):
+    @staticmethod
+    def _get_env_var_option(key):


What happened to section?

ashb · 2018-08-07T20:19:44Z

airflow/contrib/operators/dataflow_operator.py

@@ -331,7 +331,7 @@ def execute(self, context):
            self.py_file, self.py_options)


-class GoogleCloudBucketHelper():
+class GoogleCloudBucketHelper:


For py2 compat this should probablt be class GoogleCloudBucketHelper(object)

ashb · 2018-08-07T20:20:26Z

airflow/contrib/operators/oracle_to_oracle_transfer.py

@@ -52,10 +52,12 @@ def __init__(
            destination_table,
            oracle_source_conn_id,
            source_sql,
-            source_sql_params={},
+        source_sql_params=None,


ashb · 2018-08-07T20:21:25Z

airflow/operators/s3_to_hive_operator.py

    def _delete_top_row_and_compress(
-            self,
-            input_file_name,
+        input_file_name,


kaxil · 2018-08-07T20:25:12Z

Hi @ashb, Sorry, I need to sort couple of things here. I have added a WIP flag. Will ping you as soon as it is ready for review.

kaxil · 2018-08-07T20:37:53Z

@ashb This is now ready for review. :)

ashb · 2018-08-07T20:40:51Z

tests/core.py

@@ -2423,7 +2423,7 @@ def test_init_proxy_user(self):
 class HDFSHookTest(unittest.TestCase):
    def setUp(self):
        configuration.load_test_config()
-        os.environ['AIRFLOW_CONN_HDFS_DEFAULT'] = ('hdfs://localhost:8020')
+        os.environ['AIRFLOW_CONN_HDFS_DEFAULT'] = 'hdfs://localhost:8020'


(Not related to this Pr, but we have HDFS tests in core.py? o_O)

- Dictionary creation should be written by dictionary literal - Python’s default arguments are evaluated once when the function is defined, not each time the function is called (like it is in say, Ruby). This means that if you use a mutable default argument and mutate it, you will and have mutated that object for all future calls to the function as well. - Functions calling sets which can be replaced by set literal are now replaced by set literal - Replace list literals - Some of the static methods haven't been set static - Remove redundant parentheses

feng-tao · 2018-08-07T22:48:03Z

airflow/contrib/operators/oracle_to_azure_data_lake_transfer.py

+import os
+
+
+class OracleToAzureDataLakeTransfer(BaseOperator):


what has been changed to this file?

Line endings... :'(

kaxil · 2018-08-07T23:01:41Z

airflow/contrib/operators/oracle_to_azure_data_lake_transfer.py

+            quoting=csv.QUOTE_MINIMAL,
+            *args, **kwargs):
+        super(OracleToAzureDataLakeTransfer, self).__init__(*args, **kwargs)
+        if sql_params is None:


@feng-tao This line is changed. Git shows entire file has changed due to termination of line. Check https://stackoverflow.com/questions/19593909/git-diff-sees-whole-file-as-changed-when-its-not for more info

feng-tao · 2018-08-07T23:05:06Z

lgtm

codecov-io · 2018-08-07T23:17:56Z

Codecov Report

Merging #3714 into master will increase coverage by <.01%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master    #3714      +/-   ##
==========================================
+ Coverage   77.56%   77.56%   +<.01%     
==========================================
  Files         204      204              
  Lines       15768    15770       +2     
==========================================
+ Hits        12230    12232       +2     
  Misses       3538     3538

Impacted Files	Coverage Δ
airflow/www_rbac/forms.py	`100% <ø> (ø)`	⬆️
airflow/jobs.py	`82.76% <ø> (ø)`	⬆️
airflow/www/views.py	`68.88% <0%> (ø)`	⬆️
airflow/www_rbac/views.py	`72.85% <0%> (ø)`	⬆️
airflow/utils/log/gcs_task_handler.py	`0% <0%> (ø)`	⬆️
airflow/operators/hive_stats_operator.py	`0% <0%> (ø)`	⬆️
airflow/executors/dask_executor.py	`2% <0%> (ø)`	⬆️
airflow/operators/check_operator.py	`58.26% <100%> (ø)`	⬆️
airflow/hooks/S3_hook.py	`95% <100%> (+0.17%)`	⬆️
airflow/utils/cli.py	`100% <100%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9fecba...c3b5ea1. Read the comment docs.

xnuinside · 2018-08-08T02:18:55Z

airflow/contrib/hooks/bigquery_hook.py

@@ -238,6 +238,8 @@ def create_empty_table(self,

        :return:
        """
+        if time_partitioning is None:
+            time_partitioning = dict()


@kaxil seems it is missed

What do you mean? Setting the {} in the arguments is bad practice by the way Python creates these objects.

@Fokko No no, it is a bad practise to use {} or dict() as a default argument. Reference: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

The above change is because we had the below code:

which I changed to the following:

@kaxil but why dict() instead of {}? just want to understand for my self
as I know {} more efficient https://stackoverflow.com/questions/664118/whats-the-difference-between-dict-and. In official docs cannot see any recommendations to use dict() instead {} https://docs.python.org/3.6/library/stdtypes.html#dict
and in standard library used {} not dict()

{} would probably be better in hindsight, but there's not much in it. On my laptop:

In [5]: %timeit {} 47.2 ns ± 2.3 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) In [6]: %timeit dict() 168 ns ± 1.77 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

so yes dict() is 3 times as "slow" as {}, but neither is particularly slow.

@xnuinside Hi, there is no specific reason for me to use {} compared to dict(). The point for this change as I mentioned earlier was to remove an empty dictionary from default arguments.

And as Ash pointed out {} dict literal is faster that dict constructor dict() but there is not huge difference as the dict builds up. Check out the below links for more detailed read:

https://doughellmann.com/blog/2012/11/12/the-performance-impact-of-using-dict-instead-of-in-cpython-2-7-2/

https://stackoverflow.com/questions/6610606/is-there-a-difference-between-using-a-dict-literal-and-a-dict-constructor

@kaxil , @ashb , thx!

Fokko

Nice work @kaxil

Fokko · 2018-08-08T07:20:01Z

airflow/contrib/hooks/bigquery_hook.py

@@ -238,6 +238,8 @@ def create_empty_table(self,

        :return:
        """
+        if time_partitioning is None:
+            time_partitioning = dict()


What do you mean? Setting the {} in the arguments is bad practice by the way Python creates these objects.

Fokko · 2018-08-08T07:38:12Z

airflow/contrib/operators/mongo_to_s3.py

@@ -105,7 +106,8 @@ def _stringify(self, iterable, joinable='\n'):
            [json.dumps(doc, default=json_util.default) for doc in iterable]
        )

-    def transform(self, docs):
+    @staticmethod


Nit, I always put static function above the non-static one in the class order

+1, I would also prefer static methods above instance methods

Fokko · 2018-08-08T07:38:28Z

airflow/contrib/operators/oracle_to_azure_data_lake_transfer.py

+import os
+
+
+class OracleToAzureDataLakeTransfer(BaseOperator):


Line endings... :'(

- Dictionary creation should be written by dictionary literal - Python’s default arguments are evaluated once when the function is defined, not each time the function is called (like it is in say, Ruby). This means that if you use a mutable default argument and mutate it, you will and have mutated that object for all future calls to the function as well. - Functions calling sets which can be replaced by set literal are now replaced by set literal - Replace list literals - Some of the static methods haven't been set static - Remove redundant parentheses

ashb requested changes Aug 7, 2018

View reviewed changes

kaxil force-pushed the code-cleaning branch from e0bba23 to 7a09c6c Compare August 7, 2018 20:23

kaxil changed the title ~~[AIRFLOW-2867] Refactor code to conform Python standards & guidelines~~ [WIP][AIRFLOW-2867] Refactor code to conform Python standards & guidelines Aug 7, 2018

kaxil force-pushed the code-cleaning branch 2 times, most recently from 74871c3 to c69025e Compare August 7, 2018 20:37

kaxil changed the title ~~[WIP][AIRFLOW-2867] Refactor code to conform Python standards & guidelines~~ [AIRFLOW-2867] Refactor code to conform Python standards & guidelines Aug 7, 2018

ashb reviewed Aug 7, 2018

View reviewed changes

ashb approved these changes Aug 7, 2018

View reviewed changes

kaxil force-pushed the code-cleaning branch from c69025e to 94d0287 Compare August 7, 2018 20:59

kaxil force-pushed the code-cleaning branch from 94d0287 to c3b5ea1 Compare August 7, 2018 22:22

feng-tao reviewed Aug 7, 2018

View reviewed changes

kaxil commented Aug 7, 2018

View reviewed changes

feng-tao merged commit 120f485 into apache:master Aug 7, 2018

xnuinside reviewed Aug 8, 2018

View reviewed changes

Fokko reviewed Aug 8, 2018

View reviewed changes

kaxil deleted the code-cleaning branch August 8, 2018 08:34

kaxil added a commit that referenced this pull request Nov 29, 2018

[AIRFLOW-2867] Refactor Code to conform standards (#3714)

08a6c72

kaxil added a commit that referenced this pull request Jan 9, 2019

[AIRFLOW-2867] Refactor Code to conform standards (#3714)

5ddfd8b

ashb pushed a commit to ashb/airflow that referenced this pull request Jan 10, 2019

[AIRFLOW-2867] Refactor Code to conform standards (apache#3714)

e0f7cb3

cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Jan 23, 2019

Refactor Code to conform standards (apache#3714)

ff51a05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-2867] Refactor code to conform Python standards & guidelines #3714

[AIRFLOW-2867] Refactor code to conform Python standards & guidelines #3714

kaxil commented Aug 7, 2018

ashb Aug 7, 2018

ashb Aug 7, 2018

ashb Aug 7, 2018

ashb Aug 7, 2018

kaxil commented Aug 7, 2018

kaxil commented Aug 7, 2018

ashb Aug 7, 2018

feng-tao Aug 7, 2018

Fokko Aug 8, 2018

kaxil Aug 7, 2018

feng-tao commented Aug 7, 2018

codecov-io commented Aug 7, 2018

xnuinside Aug 8, 2018

Fokko Aug 8, 2018

kaxil Aug 8, 2018

xnuinside Aug 8, 2018

ashb Aug 8, 2018

kaxil Aug 8, 2018 •

edited

Loading

xnuinside Aug 8, 2018

Fokko left a comment

Fokko Aug 8, 2018

Fokko Aug 8, 2018

feng-tao Aug 8, 2018

Fokko Aug 8, 2018

[AIRFLOW-2867] Refactor code to conform Python standards & guidelines #3714

[AIRFLOW-2867] Refactor code to conform Python standards & guidelines #3714

Conversation

kaxil commented Aug 7, 2018

Jira

Description

Tests

Commits

Documentation

Code Quality

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil commented Aug 7, 2018

kaxil commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-tao commented Aug 7, 2018

codecov-io commented Aug 7, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxil Aug 8, 2018 •

edited

Loading