Skip to content

Attribute list truncation is misleading, and hides differences past the fifth element #371

Description

@danielfromearth

Summary

_value_to_comparable_str in getters.py (the shared normalizer introduced by
PR #344; previously the body of get_attribute_value_as_str) renders any non-string
iterable as at most five elements:

return "[" + ", ".join([str(x) for x in list(value)[:5]]) + ", ..." + "]"

Two separate problems, of very different sizes.

1. The ellipsis is unconditional (small)

", ..." is appended whether or not anything was actually truncated, so a
three-element attribute renders as [NASA, JPL, GSFC, ...] and implies elements
that do not exist. PR #344's new test now pins this behavior:

assert result["sources"] == "[NASA, JPL, GSFC, ...]"

Fix: only append the ellipsis when len(value) > 5. The pinned assertion above
needs updating in the same change.

2. Differences past the fifth element are undetectable (larger)

This is the pre-existing TODO in that function. Because the same truncated
string is what gets compared, two attributes that agree on their first five
elements and differ afterwards are reported as identical. The truncation is a
display concern that is silently doing double duty as the comparison value.

Fix requires separating the two: compare full values, display truncated ones.
Outputter.side_by_side currently derives its "are these different?" answer from
the strings it prints, so this needs either a way to pass display and comparison
values separately, or the comparison to happen before truncation and be handed in.

Worth noting that CONTRIBUTING.md's review checklist includes "There are no TODOs"
(line 155), and this is the only TODO left anywhere in ncompare/. Closing
this issue lets it be deleted rather than carried indefinitely.

Verification

  • Part 1: an attribute with fewer than five elements renders without an ellipsis;
    one with more than five keeps it.
  • Part 2: two files whose attribute agrees on elements 1–5 and differs at element 6
    must be reported as differing. This test fails today.
  • Both parts change rendering, so the golden files move, and part 2 changes
    difference counts (more differences become detectable), so
    EXPECTED_ATL06_DIFFERENCES will move. Report the number the suite produces and
    state why, per the usual practice.

Context

Surfaced while reviewing PR #344, which relocated this code into the shared
normalizer without changing its behavior. Part 1 is nearly trivial; part 2 is a
design change to how comparison and display are separated, so these may be worth
splitting if they get picked up at different times.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions