You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "eumiro (via GitHub)" <gi...@apache.org> on 2023/08/16 19:26:48 UTC
[GitHub] [airflow] eumiro opened a new pull request, #33455: Replace strftime with f-strings where nicer
eumiro opened a new pull request, #33455:
URL: https://github.com/apache/airflow/pull/33455
(no comment)
--
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
[GitHub] [airflow] uranusjr commented on pull request #33455: Replace strftime with f-strings where nicer
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #33455:
URL: https://github.com/apache/airflow/pull/33455#issuecomment-1681616349
I’m not particular sure about this, strftime is more obvious and (subjectively) easier to read. The f-string format can be beneficial if the formatted date string is going into another f-string (as in some cases here) but not when it’s on its own.
--
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
[GitHub] [airflow] potiuk merged pull request #33455: Replace strftime with f-strings where nicer
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #33455:
URL: https://github.com/apache/airflow/pull/33455
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1299732250
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -206,9 +206,7 @@ def _clean_date(value: datetime | None) -> str:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#_reserved_characters
"""
- if value is None:
- return ""
- return value.strftime("%Y_%m_%dT%H_%M_%S_%f")
+ return value.strftime("%Y_%m_%dT%H_%M_%S_%f") if value else ""
Review Comment:
Doesn’t seem particularly necessary to me, we’re not playing code golf…
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -206,9 +206,7 @@ def _clean_date(value: datetime | None) -> str:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#_reserved_characters
"""
- if value is None:
- return ""
- return value.strftime("%Y_%m_%dT%H_%M_%S_%f")
+ return value.strftime("%Y_%m_%dT%H_%M_%S_%f") if value else ""
Review Comment:
Doesn’t seem particularly necessary to me, we’re not playing code golf…
--
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
[GitHub] [airflow] eumiro commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1300507629
##########
tests/providers/apache/kylin/operators/test_kylin_cube.py:
##########
@@ -37,8 +37,8 @@ class TestKylinCubeOperator:
"project": "learn_kylin",
"cube": "kylin_sales_cube",
"command": "build",
- "start_time": datetime(2012, 1, 2, 0, 0).strftime("%s") + "000",
- "end_time": datetime(2012, 1, 3, 0, 0).strftime("%s") + "000",
+ "start_time": f"{datetime(2012, 1, 2, 0, 0):%s}000",
+ "end_time": f"{datetime(2012, 1, 3, 0, 0):%s}000",
Review Comment:
Changed it to a more explicit version.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1299733628
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -206,9 +206,7 @@ def _clean_date(value: datetime | None) -> str:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#_reserved_characters
"""
- if value is None:
- return ""
- return value.strftime("%Y_%m_%dT%H_%M_%S_%f")
+ return value.strftime("%Y_%m_%dT%H_%M_%S_%f") if value else ""
Review Comment:
Same for other fast return checks below.
--
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
[GitHub] [airflow] potiuk commented on pull request #33455: Replace strftime with f-strings where nicer
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33455:
URL: https://github.com/apache/airflow/pull/33455#issuecomment-1685353375
> I’m not particular sure about this, strftime is more obvious and (subjectively) easier to read. The f-string format can be beneficial if the formatted date string is going into another f-string (as in some cases here) but not when it’s on its own.
I really like conciseness of the way it's done now. It very nicely focuses the concerns (date + format) in one place within the larger f-string. @uranusjr -> is that good for you as well?
--
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
[GitHub] [airflow] eumiro commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1300507868
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -206,9 +206,7 @@ def _clean_date(value: datetime | None) -> str:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#_reserved_characters
"""
- if value is None:
- return ""
- return value.strftime("%Y_%m_%dT%H_%M_%S_%f")
+ return value.strftime("%Y_%m_%dT%H_%M_%S_%f") if value else ""
Review Comment:
Reverted all such cases.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1299736279
##########
tests/providers/apache/kylin/operators/test_kylin_cube.py:
##########
@@ -37,8 +37,8 @@ class TestKylinCubeOperator:
"project": "learn_kylin",
"cube": "kylin_sales_cube",
"command": "build",
- "start_time": datetime(2012, 1, 2, 0, 0).strftime("%s") + "000",
- "end_time": datetime(2012, 1, 3, 0, 0).strftime("%s") + "000",
+ "start_time": f"{datetime(2012, 1, 2, 0, 0):%s}000",
+ "end_time": f"{datetime(2012, 1, 3, 0, 0):%s}000",
Review Comment:
It’s probably better to just convert these to literals with a comment, going through datetime just to convert into a datetime is a bit over the top.
##########
tests/providers/apache/kylin/operators/test_kylin_cube.py:
##########
@@ -37,8 +37,8 @@ class TestKylinCubeOperator:
"project": "learn_kylin",
"cube": "kylin_sales_cube",
"command": "build",
- "start_time": datetime(2012, 1, 2, 0, 0).strftime("%s") + "000",
- "end_time": datetime(2012, 1, 3, 0, 0).strftime("%s") + "000",
+ "start_time": f"{datetime(2012, 1, 2, 0, 0):%s}000",
+ "end_time": f"{datetime(2012, 1, 3, 0, 0):%s}000",
Review Comment:
It’s probably better to just convert these to literals with a comment, going through datetime just to convert into a datetime is a bit over the top.
--
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
[GitHub] [airflow] eumiro commented on pull request #33455: Replace strftime with f-strings where nicer
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on PR #33455:
URL: https://github.com/apache/airflow/pull/33455#issuecomment-1682687537
@uranusjr Reduced to being part of larger f-strings.
--
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
[GitHub] [airflow] uranusjr commented on a diff in pull request #33455: Replace strftime with f-strings where nicer
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33455:
URL: https://github.com/apache/airflow/pull/33455#discussion_r1299733628
##########
airflow/providers/elasticsearch/log/es_task_handler.py:
##########
@@ -206,9 +206,7 @@ def _clean_date(value: datetime | None) -> str:
https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html#_reserved_characters
"""
- if value is None:
- return ""
- return value.strftime("%Y_%m_%dT%H_%M_%S_%f")
+ return value.strftime("%Y_%m_%dT%H_%M_%S_%f") if value else ""
Review Comment:
Same for other early return checks below.
--
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