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.
Summary
_value_to_comparable_stringetters.py(the shared normalizer introduced byPR #344; previously the body of
get_attribute_value_as_str) renders any non-stringiterable as at most five elements:
Two separate problems, of very different sizes.
1. The ellipsis is unconditional (small)
", ..."is appended whether or not anything was actually truncated, so athree-element attribute renders as
[NASA, JPL, GSFC, ...]and implies elementsthat do not exist. PR #344's new test now pins this behavior:
Fix: only append the ellipsis when
len(value) > 5. The pinned assertion aboveneeds updating in the same change.
2. Differences past the fifth element are undetectable (larger)
This is the pre-existing
TODOin that function. Because the same truncatedstring 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_sidecurrently derives its "are these different?" answer fromthe 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
TODOleft anywhere inncompare/. Closingthis issue lets it be deleted rather than carried indefinitely.
Verification
one with more than five keeps it.
must be reported as differing. This test fails today.
difference counts (more differences become detectable), so
EXPECTED_ATL06_DIFFERENCESwill move. Report the number the suite produces andstate 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.