You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jlaneve (via GitHub)" <gi...@apache.org> on 2023/10/31 02:24:13 UTC
[PR] Switch from Black to Ruff [airflow]
jlaneve opened a new pull request, #35287:
URL: https://github.com/apache/airflow/pull/35287
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377925565
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for dag_id in dags_to_delete_query]
Review Comment:
Oh, review/fix race condition 🍺
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for dag_id in dags_to_delete_query]
Review Comment:
Oh, review/fix race condition 🍺
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377918877
##########
airflow/models/dag.py:
##########
@@ -3618,7 +3618,7 @@ def get_paused_dag_ids(dag_ids: list[str], session: Session = NEW_SESSION) -> se
.where(DagModel.dag_id.in_(dag_ids))
)
- paused_dag_ids = {paused_dag_id for paused_dag_id, in paused_dag_ids}
+ paused_dag_ids = {paused_dag_id for paused_dag_id in paused_dag_ids}
Review Comment:
Same here, now a set of tuples vs str.
(last one I'll comment on, but I expect all of there were intentional)
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for dag_id in dags_to_delete_query]
Review Comment:
This wasn't a typo. It's a single item tuple from sqla.
```
Before: ['simple']
After: [('simple',)]
```
Also, otherwise we could just do `list(dags_to_delete_query)`.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jlaneve (via GitHub)" <gi...@apache.org>.
jlaneve commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377712293
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for (dag_id,) in dags_to_delete_query]
Review Comment:
yeah this looks like it was a typo before - I've gone through and removed all cases like 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jlaneve (via GitHub)" <gi...@apache.org>.
jlaneve commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377711652
##########
airflow/providers/google/cloud/transfers/bigquery_to_mssql.py:
##########
@@ -63,8 +63,7 @@ def __init__(
if mssql_table is not None:
warnings.warn(
# fmt: off
- "The `mssql_table` parameter has been deprecated. "
- "Use `target_table_name` instead.",
+ "The `mssql_table` parameter has been deprecated. Use `target_table_name` instead.",
# fmt: on
Review Comment:
Agreed. I've removed this one for now because it's no longer used
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #35287:
URL: https://github.com/apache/airflow/pull/35287
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "dhruvmanila (via GitHub)" <gi...@apache.org>.
dhruvmanila commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1378344674
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for (dag_id,) in dags_to_delete_query]
Review Comment:
For reference: https://docs.astral.sh/ruff/formatter/black/#single-element-tuples-are-always-parenthesized
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377553765
##########
tests/providers/common/sql/operators/test_sql.py:
##########
@@ -131,15 +131,15 @@ class TestColumnCheckOperator:
UNION ALL
SELECT 'X' AS col_name, 'distinct_check' AS check_type, X_distinct_check AS check_result
FROM (SELECT COUNT(DISTINCT(X)) AS X_distinct_check FROM test_table WHERE Y > 1) AS sq
- """ # noqa 501
+ """ # 501
Review Comment:
huh, this isn't right surely?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jlaneve (via GitHub)" <gi...@apache.org>.
jlaneve commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377708296
##########
tests/system/providers/docker/example_docker.py:
##########
@@ -54,9 +54,7 @@
(
# TEST BODY
- t1
- >> [t2, t3]
- >> t4
+ t1 >> [t2, t3] >> t4
Review Comment:
Interesting, I do actually prefer the single line version. Going to resolve this because it's more stylistic and I've seen it both ways
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jlaneve (via GitHub)" <gi...@apache.org>.
jlaneve commented on PR #35287:
URL: https://github.com/apache/airflow/pull/35287#issuecomment-1787633576
note that I added back inline tuples - turns out they were actually correct. see https://github.com/apache/airflow/actions/runs/6708119087/job/18230292697
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #35287:
URL: https://github.com/apache/airflow/pull/35287#issuecomment-1788142100
The current constraints failure is unrelated and affects other PRs too, example https://github.com/apache/airflow/pull/35312 , merging this PR
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "jlaneve (via GitHub)" <gi...@apache.org>.
jlaneve commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377709828
##########
tests/providers/common/sql/operators/test_sql.py:
##########
@@ -131,15 +131,15 @@ class TestColumnCheckOperator:
UNION ALL
SELECT 'X' AS col_name, 'distinct_check' AS check_type, X_distinct_check AS check_result
FROM (SELECT COUNT(DISTINCT(X)) AS X_distinct_check FROM test_table WHERE Y > 1) AS sq
- """ # noqa 501
+ """ # 501
Review Comment:
I'm not too sure why this was here before... it didn't seem to be doing anything
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #35287:
URL: https://github.com/apache/airflow/pull/35287#issuecomment-1786890962
Looks like it works. Care to undraft it ?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377552986
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for (dag_id,) in dags_to_delete_query]
Review Comment:
This might be auto-generated? Is there a rule around it @jlaneve ?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377671630
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for (dag_id,) in dags_to_delete_query]
Review Comment:
I thought ruff just explicitly turn into the tuple because not all cases could be resolved.
Like this one
```python
a = 42,
```
In could be a typo (more common problem) or just shorthand for 1-element tuple
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Switch from Black to Ruff formatter [airflow]
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on code in PR #35287:
URL: https://github.com/apache/airflow/pull/35287#discussion_r1377461406
##########
.pre-commit-config.yaml:
##########
@@ -181,13 +174,16 @@ repos:
pass_filenames: false
require_serial: true
- repo: https://github.com/astral-sh/ruff-pre-commit
- rev: v0.0.292
+ rev: v0.1.3
hooks:
# Since ruff makes use of multiple cores we _purposefully_ don't run this in docker so it can use the
# host CPU to it's fullest
- id: ruff
- name: ruff
- args: [ --fix ]
+ name: ruff-lint
+ args: [--fix]
+ exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py
+ - id: ruff-format
+ name: ruff-format
exclude: ^.*/.*_vendor/|^tests/dags/test_imports.py
Review Comment:
I guess it should be `^.*/.*_vendor/|^airflow/contrib/`
Seems like that is the reason why a lot of changes in to the legacy `airflow.contrib`
##########
tests/www/views/test_views_base.py:
##########
@@ -88,7 +88,7 @@ def heartbeat_too_slow():
state="running",
latest_heartbeat=last_heartbeat,
)
- SchedulerJobRunner(job=job),
+ (SchedulerJobRunner(job=job),)
Review Comment:
```suggestion
SchedulerJobRunner(job=job)
```
##########
tests/providers/common/sql/operators/test_sql.py:
##########
@@ -131,15 +131,15 @@ class TestColumnCheckOperator:
UNION ALL
SELECT 'X' AS col_name, 'distinct_check' AS check_type, X_distinct_check AS check_result
FROM (SELECT COUNT(DISTINCT(X)) AS X_distinct_check FROM test_table WHERE Y > 1) AS sq
- """ # noqa 501
+ """ # 501
Review Comment:
```suggestion
"""
```
##########
tests/system/providers/docker/example_docker.py:
##########
@@ -54,9 +54,7 @@
(
# TEST BODY
- t1
- >> [t2, t3]
- >> t4
+ t1 >> [t2, t3] >> t4
Review Comment:
Personal I don't like this behaviour in brackets. But I guess nothing we could do here?
##########
airflow/models/dag.py:
##########
@@ -3618,7 +3618,7 @@ def get_paused_dag_ids(dag_ids: list[str], session: Session = NEW_SESSION) -> se
.where(DagModel.dag_id.in_(dag_ids))
)
- paused_dag_ids = {paused_dag_id for paused_dag_id, in paused_dag_ids}
+ paused_dag_ids = {paused_dag_id for (paused_dag_id,) in paused_dag_ids}
Review Comment:
```suggestion
paused_dag_ids = {paused_dag_id for paused_dag_id in paused_dag_ids}
```
##########
tests/www/views/test_views_base.py:
##########
@@ -68,7 +68,7 @@ def heartbeat_healthy():
state="running",
latest_heartbeat=last_heartbeat,
)
- SchedulerJobRunner(job=job),
+ (SchedulerJobRunner(job=job),)
Review Comment:
Seems like it would be easier to found extra comma in the end, which turned into tuple.
There is no critical here because we do not assign anything, but better to fix it
```suggestion
SchedulerJobRunner(job=job)
```
##########
tests/providers/common/sql/operators/test_sql.py:
##########
@@ -131,15 +131,15 @@ class TestColumnCheckOperator:
UNION ALL
SELECT 'X' AS col_name, 'distinct_check' AS check_type, X_distinct_check AS check_result
FROM (SELECT COUNT(DISTINCT(X)) AS X_distinct_check FROM test_table WHERE Y > 1) AS sq
- """ # noqa 501
+ """ # 501
correct_generate_sql_query_with_partition_and_where = """
SELECT 'X' AS col_name, 'null_check' AS check_type, X_null_check AS check_result
FROM (SELECT SUM(CASE WHEN X IS NULL THEN 1 ELSE 0 END) AS X_null_check FROM test_table WHERE Y > 1 AND Z < 100) AS sq
UNION ALL
SELECT 'X' AS col_name, 'distinct_check' AS check_type, X_distinct_check AS check_result
FROM (SELECT COUNT(DISTINCT(X)) AS X_distinct_check FROM test_table WHERE Y > 1) AS sq
- """ # noqa 501
+ """ # 501
Review Comment:
```suggestion
"""
```
##########
tests/providers/google/cloud/hooks/test_datacatalog.py:
##########
@@ -65,9 +65,9 @@
TEST_NAME: str = "test-name"
TEST_TAG_ID: str = "test-tag-id"
TEST_LOCATION_PATH: str = f"projects/{{}}/locations/{TEST_LOCATION}"
-TEST_ENTRY_PATH: str = (
- f"projects/{{}}/locations/{TEST_LOCATION}/entryGroups/{TEST_ENTRY_GROUP_ID}/entries/{TEST_ENTRY_ID}"
-)
+TEST_ENTRY_PATH: (
+ str
+) = f"projects/{{}}/locations/{TEST_LOCATION}/entryGroups/{TEST_ENTRY_GROUP_ID}/entries/{TEST_ENTRY_ID}"
Review Comment:
That it also looks less readable rather than `black` formatting
##########
airflow/www/views.py:
##########
@@ -2447,7 +2447,7 @@ def _mark_dagrun_state_as_queued(
)
)
- completed_tis_ids = [task_id for task_id, in existing_tis]
+ completed_tis_ids = [task_id for (task_id,) in existing_tis]
Review Comment:
```suggestion
completed_tis_ids = [task_id for task_id in existing_tis]
```
##########
airflow/api/common/delete_dag.py:
##########
@@ -72,7 +72,7 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session: Session =
)
)
- dags_to_delete = [dag_id for dag_id, in dags_to_delete_query]
+ dags_to_delete = [dag_id for (dag_id,) in dags_to_delete_query]
Review Comment:
```suggestion
dags_to_delete = [dag_id for dag_id in dags_to_delete_query]
```
##########
airflow/www/views.py:
##########
@@ -929,7 +929,7 @@ def index(self):
dagtags = session.execute(select(func.distinct(DagTag.name)).order_by(DagTag.name)).all()
tags = [
{"name": name, "selected": bool(arg_tags_filter and name in arg_tags_filter)}
- for name, in dagtags
+ for (name,) in dagtags
Review Comment:
```suggestion
for name in dagtags
```
##########
airflow/providers/google/cloud/transfers/bigquery_to_mssql.py:
##########
@@ -63,8 +63,7 @@ def __init__(
if mssql_table is not None:
warnings.warn(
# fmt: off
- "The `mssql_table` parameter has been deprecated. "
- "Use `target_table_name` instead.",
+ "The `mssql_table` parameter has been deprecated. Use `target_table_name` instead.",
# fmt: on
Review Comment:
Not related to this PR but we should found as follow-up all `# fmt: off` and `# fmt: on` and remove it from codebase. I guess most of them (if not all) not required
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org