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/09/05 19:48:25 UTC
[GitHub] [airflow] eumiro opened a new pull request, #34118: Refactor: Simplify string generation
eumiro opened a new pull request, #34118:
URL: https://github.com/apache/airflow/pull/34118
(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 a diff in pull request #34118: Refactor: Simplify string generation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1318222825
##########
airflow/providers/amazon/aws/operators/sagemaker.py:
##########
@@ -166,9 +167,8 @@ def hook(self):
def path_to_s3_dataset(path) -> Dataset:
from openlineage.client.run import Dataset
- path = path.replace("s3://", "")
- split_path = path.split("/")
- return Dataset(namespace=f"s3://{split_path[0]}", name="/".join(split_path[1:]), facets={})
+ split = urlsplit(path)
+ return Dataset(namespace=f"{split.scheme}://{split.netloc}", name=split.path, facets={})
Review Comment:
Would the path contain query parameters like `?foo=bar&x`? Not familiar with this, but using `split.path` would loose those. We might need `urlunsplit(split._replace(scheme="", netloc="")).lstrip("/")` instead (getting rid of the leading slash to match the current behaviour)
--
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 #34118: Refactor: Simplify string generation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1316528170
##########
airflow/providers/amazon/aws/operators/sagemaker.py:
##########
@@ -166,9 +166,8 @@ def hook(self):
def path_to_s3_dataset(path) -> Dataset:
from openlineage.client.run import Dataset
- path = path.replace("s3://", "")
- split_path = path.split("/")
- return Dataset(namespace=f"s3://{split_path[0]}", name="/".join(split_path[1:]), facets={})
+ namespace, _, name = path[len("s3://") :].partition("/")
+ return Dataset(namespace=f"s3://{namespace}", name=name, facets={})
Review Comment:
I wonder if it’d be easier to just use `urllib.parse.urlsplit` here
##########
airflow/providers/amazon/aws/operators/sagemaker.py:
##########
@@ -166,9 +166,8 @@ def hook(self):
def path_to_s3_dataset(path) -> Dataset:
from openlineage.client.run import Dataset
- path = path.replace("s3://", "")
- split_path = path.split("/")
- return Dataset(namespace=f"s3://{split_path[0]}", name="/".join(split_path[1:]), facets={})
+ namespace, _, name = path[len("s3://") :].partition("/")
+ return Dataset(namespace=f"s3://{namespace}", name=name, facets={})
Review Comment:
I wonder if it’d be easier to just use `urllib.parse.urlsplit` here
--
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] dwreeves commented on a diff in pull request #34118: Refactor: Simplify string generation
Posted by "dwreeves (via GitHub)" <gi...@apache.org>.
dwreeves commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1319223549
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -122,13 +122,7 @@ def _create_pod_id(
"""
if not (dag_id or task_id):
raise ValueError("Must supply either dag_id or task_id.")
- name = ""
- if dag_id:
- name += dag_id
- if task_id:
- if name:
- name += "-"
- name += task_id
+ name = f"{dag_id or ''}-{task_id or ''}"
Review Comment:
Strictly speaking this is not a refactor as the `-` is always included in the new version, but not the old 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 #34118: Refactor: Simplify string generation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1316560874
##########
airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -173,8 +173,8 @@ def get_conn(self) -> BlobServiceClient:
# provided the DNS name without the https:// prefix.
# Azure storage account name can only be 3 to 24 characters in length
# https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#storage-account-name
- acc_name = account_url.split(".")[0][:24]
- account_url = f"https://{acc_name}." + ".".join(account_url.split(".")[1:])
+ acc_name, acc_url = account_url.split(".", 1)
+ account_url = f"https://{acc_name[:24]}.{acc_url}"
Review Comment:
This logic would produce a URL with a trailing dot if the original URL does not contain any dots. But the original implementation already has this problem so I’m not sure if we want to fix it here.
##########
airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -173,8 +173,8 @@ def get_conn(self) -> BlobServiceClient:
# provided the DNS name without the https:// prefix.
# Azure storage account name can only be 3 to 24 characters in length
# https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#storage-account-name
- acc_name = account_url.split(".")[0][:24]
- account_url = f"https://{acc_name}." + ".".join(account_url.split(".")[1:])
+ acc_name, acc_url = account_url.split(".", 1)
+ account_url = f"https://{acc_name[:24]}.{acc_url}"
Review Comment:
This logic would produce a URL with a trailing dot if the original URL does not contain any dots. But the original implementation already has this problem so I’m not sure if we want to fix it here.
--
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 #34118: Refactor: Simplify string generation
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #34118:
URL: https://github.com/apache/airflow/pull/34118
--
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 #34118: Refactor: Simplify string generation
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1317591718
##########
airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -173,8 +173,8 @@ def get_conn(self) -> BlobServiceClient:
# provided the DNS name without the https:// prefix.
# Azure storage account name can only be 3 to 24 characters in length
# https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#storage-account-name
- acc_name = account_url.split(".")[0][:24]
- account_url = f"https://{acc_name}." + ".".join(account_url.split(".")[1:])
+ acc_name, acc_url = account_url.split(".", 1)
+ account_url = f"https://{acc_name[:24]}.{acc_url}"
Review Comment:
Since you mentioned it, I've suggested a fix.
--
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 #34118: Refactor: Simplify string generation
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1318228119
##########
scripts/in_container/run_migration_reference.py:
##########
@@ -135,22 +135,18 @@ def update_docs(revisions: Iterable[Script]):
)
-def num_to_prefix(idx: int) -> str:
- return f"000{idx+1}"[-4:] + "_"
-
-
def ensure_mod_prefix(mod_name, idx, version):
- prefix = num_to_prefix(idx) + "_".join(version) + "_"
+ parts = [f"{idx + 1:04}"] + version
Review Comment:
```suggestion
parts = [f"{idx + 1:04}", *version]
```
##########
airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -173,8 +173,8 @@ def get_conn(self) -> BlobServiceClient:
# provided the DNS name without the https:// prefix.
# Azure storage account name can only be 3 to 24 characters in length
# https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#storage-account-name
- acc_name = account_url.split(".")[0][:24]
- account_url = f"https://{acc_name}." + ".".join(account_url.split(".")[1:])
+ acc_name, acc_url = account_url.partition(".")
+ account_url = f"https://{acc_name[:24]}.{acc_url}".rstrip(".")
Review Comment:
```suggestion
acc_name, sep, acc_url = account_url.partition(".")
account_url = f"https://{acc_name[:24]}{sep}{acc_url}"
```
##########
airflow/providers/microsoft/azure/hooks/wasb.py:
##########
@@ -586,8 +586,8 @@ async def get_async_conn(self) -> AsyncBlobServiceClient:
# provided the DNS name without the https:// prefix.
# Azure storage account name can only be 3 to 24 characters in length
# https://learn.microsoft.com/en-us/azure/storage/common/storage-account-overview#storage-account-name
- acc_name = account_url.split(".")[0][:24]
- account_url = f"https://{acc_name}." + ".".join(account_url.split(".")[1:])
+ acc_name, acc_url = account_url.split(".", 1)
+ account_url = f"https://{acc_name[:24]}.{acc_url}"
Review Comment:
```suggestion
acc_name, sep, acc_url = account_url.partition(".")
account_url = f"https://{acc_name[:24]}{sep}{acc_url}"
```
--
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 #34118: Refactor: Simplify string generation
Posted by "eumiro (via GitHub)" <gi...@apache.org>.
eumiro commented on code in PR #34118:
URL: https://github.com/apache/airflow/pull/34118#discussion_r1320216030
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -122,13 +122,7 @@ def _create_pod_id(
"""
if not (dag_id or task_id):
raise ValueError("Must supply either dag_id or task_id.")
- name = ""
- if dag_id:
- name += dag_id
- if task_id:
- if name:
- name += "-"
- name += task_id
+ name = f"{dag_id or ''}-{task_id or ''}"
Review Comment:
Thanks, fixed.
--
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