You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "uranusjr (via GitHub)" <gi...@apache.org> on 2023/11/20 05:44:51 UTC
[PR] Simplify io.path [airflow]
uranusjr opened a new pull request, #35747:
URL: https://github.com/apache/airflow/pull/35747
This should not change any functionality unless I made mistakes.
--
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] Simplify io.path [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35747:
URL: https://github.com/apache/airflow/pull/35747#discussion_r1401481912
##########
airflow/io/path.py:
##########
@@ -40,21 +40,15 @@
PT = typing.TypeVar("PT", bound="ObjectStoragePath")
-default = "file"
-
class _AirflowCloudAccessor(_CloudAccessor):
__slots__ = ("_store",)
- def __init__(self, parsed_url: SplitResult | None, **kwargs: typing.Any) -> None:
- store = kwargs.pop("store", None)
- conn_id = kwargs.pop("conn_id", None)
- if store:
- self._store = store
- elif parsed_url and parsed_url.scheme:
+ def __init__(self, parsed_url: SplitResult | None, conn_id: str, **kwargs: typing.Any) -> None:
Review Comment:
Would `conn_id: str | None = None` make this more comfortable? The interface difference is not a tachnically relevant issue IMO, but making the argument optional is not a big deal to me.
--
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] Simplify io.path [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35747:
URL: https://github.com/apache/airflow/pull/35747#discussion_r1398686559
##########
airflow/io/path.py:
##########
@@ -123,20 +120,14 @@ def __new__(cls: type[PT], *args: str | os.PathLike, **kwargs: typing.Any) -> PT
url = stringify_path(other)
parsed_url: SplitResult = urlsplit(url)
- protocol: str | None = split_protocol(url)[0] or parsed_url.scheme
-
- # allow override of protocol
- protocol = kwargs.get("scheme", protocol)
- for key in ["scheme", "url"]:
- val = kwargs.pop(key, None)
- if val:
- parsed_url = parsed_url._replace(**{key: val})
+ if scheme: # allow override of protocol
+ parsed_url = parsed_url._replace(scheme=scheme)
Review Comment:
I removed `url` since it does not work:
```pycon
>>> from urllib.parse import urlsplit
>>> urlsplit("s3://foo/bar")._replace(url="")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/uranusjr/Library/PythonUp/versions/3.12/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/collections/__init__.py", line 460, in _replace
raise ValueError(f'Got unexpected field names: {list(kwds)!r}')
ValueError: Got unexpected field names: ['url']
```
Let me know if it represents some intention without a correct implementation so I can add the logic back.
--
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] Simplify io.path [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #35747:
URL: https://github.com/apache/airflow/pull/35747#discussion_r1398791878
##########
airflow/io/path.py:
##########
@@ -89,15 +83,18 @@ class ObjectStoragePath(CloudPath):
"_hash",
)
- def __new__(cls: type[PT], *args: str | os.PathLike, **kwargs: typing.Any) -> PT:
+ def __new__(
+ cls: type[PT],
+ *args: str | os.PathLike,
+ scheme: str | None = None,
Review Comment:
Again this overrides the original function spec. I'm not sure if we want to diverge.
##########
airflow/io/path.py:
##########
@@ -40,21 +40,15 @@
PT = typing.TypeVar("PT", bound="ObjectStoragePath")
-default = "file"
-
class _AirflowCloudAccessor(_CloudAccessor):
__slots__ = ("_store",)
- def __init__(self, parsed_url: SplitResult | None, **kwargs: typing.Any) -> None:
- store = kwargs.pop("store", None)
- conn_id = kwargs.pop("conn_id", None)
- if store:
- self._store = store
- elif parsed_url and parsed_url.scheme:
+ def __init__(self, parsed_url: SplitResult | None, conn_id: str, **kwargs: typing.Any) -> None:
Review Comment:
This overrides the __init__ constructor of the parent (CloudAccessor). While we are not using it in that way, I wanted to keep it the same so it is interchangeable.
--
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] Simplify io.path [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin merged PR #35747:
URL: https://github.com/apache/airflow/pull/35747
--
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] Simplify io.path [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin commented on code in PR #35747:
URL: https://github.com/apache/airflow/pull/35747#discussion_r1398787330
##########
airflow/io/path.py:
##########
@@ -123,20 +120,14 @@ def __new__(cls: type[PT], *args: str | os.PathLike, **kwargs: typing.Any) -> PT
url = stringify_path(other)
parsed_url: SplitResult = urlsplit(url)
- protocol: str | None = split_protocol(url)[0] or parsed_url.scheme
-
- # allow override of protocol
- protocol = kwargs.get("scheme", protocol)
- for key in ["scheme", "url"]:
- val = kwargs.pop(key, None)
- if val:
- parsed_url = parsed_url._replace(**{key: val})
+ if scheme: # allow override of protocol
+ parsed_url = parsed_url._replace(scheme=scheme)
Review Comment:
Not really. This is ported from upath and made to look the same as much as possible. So I haven't looked at all the angles.
--
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] Simplify io.path [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #35747:
URL: https://github.com/apache/airflow/pull/35747#discussion_r1401483042
##########
airflow/io/path.py:
##########
@@ -89,15 +83,18 @@ class ObjectStoragePath(CloudPath):
"_hash",
)
- def __new__(cls: type[PT], *args: str | os.PathLike, **kwargs: typing.Any) -> PT:
+ def __new__(
+ cls: type[PT],
+ *args: str | os.PathLike,
+ scheme: str | None = None,
Review Comment:
This does not. Since both the parent definition and this one take `**kwargs`, adding a keyword-only argument does not change the caller interface at all. The only difference is when `scheme` is not provided by the caller, an explicit argument receives a default (None in this case) while the key would be absent in `**kwargs`. But since the implementation here uses `pop()` anyway, the end result is exactly equivalent.
--
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