Skip to content

Commit 850e105

Browse files
Nick Manganelliclaude
andcommitted
docs(dataset_tools): trim filespec split comments to describe behavior only
Drop issue-tracker references, CI-contract narration, and "battle-tested"/ "error-prone" editorializing from the _file_object_path_split helper docstring, the files-list promotion comment, and the port-colon parsing tests. Comments now minimally describe existing behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeYa8sEdeLGa1VX2frvoNz
1 parent 624d8c4 commit 850e105

2 files changed

Lines changed: 15 additions & 30 deletions

File tree

src/coffea/dataset_tools/filespec.py

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,10 @@
2020

2121

2222
def _file_object_path_split(path: str) -> tuple[str, str | None]:
23-
"""Split a file path into ``(filename, object_path)``, delegating to uproot.
24-
25-
ROOT files may be specified as ``"filename:object_path"`` (e.g. ``"file.root:Events"``), but the
26-
filename itself can be an XRootD URL containing colons (a port, as in
27-
``"root://host:1094//store/f.root"``). Reimplementing that parse is error-prone, so we reuse
28-
uproot's splitter, which understands the ``root://`` scheme, ``//`` separators, ports, and
29-
Windows drive letters. coffea intentionally tracks uproot closely; the import and the behaviour
30-
we depend on are pinned by ``test_uproot_file_object_path_split_contract`` so an upstream move is
31-
flagged in CI rather than silently mis-parsing URLs.
23+
"""Split ``"filename:object_path"`` into ``(filename, object_path)``.
24+
25+
Delegates to uproot's splitter, which handles XRootD URLs whose host carries a
26+
port colon (e.g. ``"root://host:1094//store/f.root"``).
3227
"""
3328
from uproot._util import file_object_path_split
3429

@@ -497,10 +492,10 @@ def preprocess_data(cls, data: Any) -> Any:
497492
files = data.pop("files")
498493
# promote files list to dict if necessary
499494
if isinstance(files, list):
500-
# If files is a list, convert it to a dict and let it pass through the rest of the promotion logic.
501-
# Each entry may embed a ROOT object path as a trailing ":Tree" suffix, but the filename itself may
502-
# be an XRootD URL that contains colons (e.g. a port, "root://host:1094//path/f.root"). Delegate to
503-
# uproot's battle-tested URL/object-path splitter so we never split on the port colon.
495+
# Convert the files list to a dict and let it pass through the rest of the promotion logic.
496+
# Each entry may embed a ROOT object path as a trailing ":Tree" suffix; the filename itself may
497+
# be an XRootD URL that contains a port colon (e.g. "root://host:1094//path/f.root"), so use
498+
# uproot's splitter to separate filename from object path rather than splitting on the port colon.
504499
files_list = files
505500
files = {}
506501
for f in files_list:

tests/test_dataset_tools_filespec.py

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1204,8 +1204,7 @@ def test_legacy_form_to_compressed_form_migration(self):
12041204
("file.root:Dir/Tree", ("file.root", "Dir/Tree")),
12051205
# XRootD URL without a port and no object path (the "//" guard case)
12061206
("root://host//path/f.root", ("root://host//path/f.root", None)),
1207-
# XRootD URL WITH a port and no object path (issue #1578: must not
1208-
# split at the ':1094' port colon)
1207+
# XRootD URL WITH a port and no object path (must not split at the ':1094' port colon)
12091208
(
12101209
"root://host:1094//path/f.root",
12111210
("root://host:1094//path/f.root", None),
@@ -1225,18 +1224,15 @@ def test_legacy_form_to_compressed_form_migration(self):
12251224
],
12261225
)
12271226
def test_file_object_path_split(self, path, expected):
1228-
"""Regression for issue #1578: XRootD URLs with a port must not be split
1229-
at the port colon when separating filename from ROOT object path."""
1227+
"""XRootD URLs with a port are not split at the port colon when separating
1228+
the filename from a ROOT object path."""
12301229
from coffea.dataset_tools.filespec import _file_object_path_split
12311230

12321231
assert _file_object_path_split(path) == expected
12331232

12341233
def test_uproot_file_object_path_split_contract(self):
1235-
"""Pin the uproot dependency ``_file_object_path_split`` delegates to.
1236-
1237-
coffea reuses uproot's splitter rather than reimplementing XRootD/object-path
1238-
parsing; there is no in-house fallback. If uproot moves or changes the helper,
1239-
this fails loudly in CI so we adapt here, instead of silently mis-parsing URLs.
1234+
"""Pin the ``uproot._util.file_object_path_split`` behaviour that
1235+
``_file_object_path_split`` delegates to, so a change in uproot surfaces here.
12401236
"""
12411237
from uproot._util import file_object_path_split
12421238

@@ -1264,14 +1260,8 @@ def test_list_input_xrootd_url_with_port_and_object_path(self):
12641260
assert spec.object_path == "Events"
12651261

12661262
def test_list_input_xrootd_url_with_port_no_object_path(self):
1267-
"""Regression for issue #1578: an XRootD URL with a port and NO object
1268-
path (here a remote parquet directory, which forbids an object_path) must
1269-
not be split at the port colon.
1270-
1271-
Before the fix, "root://host:1094//store/data/dir.parquet" was split into
1272-
filename "root://host" with object_path "1094//store/data/dir.parquet",
1273-
which then failed validation because parquet files forbid an object_path.
1274-
"""
1263+
"""An XRootD URL with a port and no object path (here a remote parquet
1264+
directory, which forbids an object_path) is not split at the port colon."""
12751265
ds = DatasetSpec(
12761266
files=["root://host.cern.ch:1094//store/data/dir.parquet"],
12771267
metadata={},

0 commit comments

Comments
 (0)