Rr 87 version error msg

[ryanraaschCDC]

No description provided.

[Copilot]

Pull request overview

This PR updates cfa.dataops.utils.version_matcher to use packaging’s Version and SpecifierSet for selecting versions (including timestamp-like versions), and adds a dedicated test module covering expected matching and error behavior.

Changes:

• Added unit tests for version_matcher, including date-range matching and expected failure cases.
• Refactored version_matcher to normalize timestamp strings and use packaging for ordering and specifier matching.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

File	Description
tests/test_utils_version_matcher.py	Adds tests for “latest”, range selection, and expected error cases for invalid/out-of-range inputs.
cfa/dataops/utils.py	Reimplements version_matcher using packaging.Version/SpecifierSet and timestamp normalization.

Comments suppressed due to low confidence (2)

cfa/dataops/utils.py:219

• SpecifierSet(spec_normalized) will raise for a bare version like "2025-12-18T00-00-00" (no comparison operator). The previous implementation treated bare versions as equality, and your new tests also pass bare versions. Consider normalizing bare specs to an explicit equality specifier (e.g., prefix with ==) before constructing SpecifierSet.

    spec_normalized = spec.replace("T", ".").replace("-", ".")
    available_versions_normalized = [
        v.replace("T", ".").replace("-", ".") for v in available_versions
    ]
    specset = SpecifierSet(spec_normalized)

cfa/dataops/utils.py:228

• Mapping matches back to originals via available_versions_normalized.index(m) is O(n^2) and will return the first occurrence when there are duplicate normalized versions, producing incorrect results. Prefer keeping a parallel list of tuples (normalized, original) and filtering/sorting tuples so you never need index() lookups.

    # Map back to original versions
    original_matches = [
        available_versions[available_versions_normalized.index(m)] for m in matches
    ]

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

cfa/dataops/utils.py:219

• SpecifierSet(spec_normalized) will raise packaging.specifiers.InvalidSpecifier for inputs like a bare timestamp (e.g. 2025-12-18T00-00-00) because it lacks an operator. Since callers/tests pass bare timestamps, this makes version_matcher fail; consider treating bare specs as equality (prefix ==) and catching InvalidSpecifier to re-raise ValueError with the intended message.

    spec_normalized = spec.replace("T", ".").replace("-", ".")
    available_versions_normalized = [
        v.replace("T", ".").replace("-", ".") for v in available_versions
    ]
    specset = SpecifierSet(spec_normalized)

cfa/dataops/utils.py:228

• Mapping normalized matches back to originals via available_versions_normalized.index(m) is O(n^2) and returns the first occurrence only, which is incorrect if available_versions contains duplicates. Prefer building a mapping from normalized->original (or filtering a list of (normalized, original) pairs) so the correspondence is stable and linear-time.

    # Map back to original versions
    original_matches = [
        available_versions[available_versions_normalized.index(m)] for m in matches
    ]

cfa/dataops/utils.py:199

• This change removes the version_matcher docstring and return type annotation, even though it has non-trivial behavior (str | list[str] | None). Restoring documentation and an explicit return type would help callers understand what to expect (especially the new None returns when no matches exist).

def version_matcher(
    spec: str,
    available_versions: list[str],
    newest: bool | None = True,
):
    if spec == "latest":
        if not available_versions:
            return None

[damonbayer]

a few more observations

[damonbayer]

Tests could use a once-over. There is some redundancy and others that don't test what they say they do.

[damonbayer]

Thanks @ryanraaschCDC!