uranusjr commented on code in PR #76890:
URL: https://github.com/apache/airflow/pull/76890#discussion_r4535742954
##########
shared/module_loading/src/airflow_shared/module_loading/__init__.py:
##########
@@ -178,8 +178,11 @@ def _get_grouped_entry_points() -> dict[str, list[EPnD]]:
# A distribution is yielded once per sys.path entry it is found on
(e.g. RHEL/Fedora
# venvs list both lib64/ and lib/ site-packages), possibly in
different versions.
# Only the first one is importable, so it is the only one whose
entry points count.
- dist_metadata = dist.metadata
- if dist_metadata and (name := dist_metadata.get("Name")):
+ try:
+ name = dist._normalized_name # type: ignore[attr-defined]
+ except Exception:
+ name = None
+ if name:
normalized_name = _normalize_dist_name(name)
Review Comment:
This can go away, `name` is already normalized.
##########
shared/module_loading/tests/module_loading/test_entry_points.py:
##########
@@ -33,6 +33,10 @@ def __init__(
self.entry_points = entry_points
self.version = version
+ @property
+ def _normalized_name(self) -> str:
+ return self.metadata["Name"] # type: ignore[index]
Review Comment:
This is not technically correct since the name in the metadata is not
normalized. It works now due to the redundant normalize call.
##########
shared/module_loading/src/airflow_shared/module_loading/__init__.py:
##########
@@ -178,8 +178,11 @@ def _get_grouped_entry_points() -> dict[str, list[EPnD]]:
# A distribution is yielded once per sys.path entry it is found on
(e.g. RHEL/Fedora
# venvs list both lib64/ and lib/ site-packages), possibly in
different versions.
# Only the first one is importable, so it is the only one whose
entry points count.
- dist_metadata = dist.metadata
- if dist_metadata and (name := dist_metadata.get("Name")):
+ try:
+ name = dist._normalized_name # type: ignore[attr-defined]
+ except Exception:
Review Comment:
```suggestion
except (KeyError, TypeError):
```
##########
shared/module_loading/tests/module_loading/test_entry_points.py:
##########
@@ -97,6 +101,29 @@ def
test_entry_points_with_dist_same_dist_in_two_versions_returns_first_only():
]
+class MockPathDistribution:
Review Comment:
This only differs from `MockDistribution` by having `metadata` raise. A
`MockDistribution(...)` with `metadata` patched to raise would avoid a second
mock class, but either is fine.
##########
shared/module_loading/src/airflow_shared/module_loading/__init__.py:
##########
@@ -178,8 +178,11 @@ def _get_grouped_entry_points() -> dict[str, list[EPnD]]:
# A distribution is yielded once per sys.path entry it is found on
(e.g. RHEL/Fedora
# venvs list both lib64/ and lib/ site-packages), possibly in
different versions.
# Only the first one is importable, so it is the only one whose
entry points count.
- dist_metadata = dist.metadata
- if dist_metadata and (name := dist_metadata.get("Name")):
+ try:
+ name = dist._normalized_name # type: ignore[attr-defined]
+ except Exception:
+ name = None
+ if name:
normalized_name = _normalize_dist_name(name)
Review Comment:
We should also comment close to `_normalized_name` why we opt for a private
attribute.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
