fix(ecr): align image list columns to fixed widths (#195)#196
Conversation
) Closes DevopsArtFactory#195. `ECRImage.DisplayTitle` rendered tag/digest/pushed/size/status as a space-padded but variable-width string. When tags had different lengths the digest column started at a different visual position per row, making the list hard to scan. Pin each column to a fixed rune-width and pad/truncate accordingly: - tag 30 (truncates with '…' on overflow) - digest 19 (shortDigest already produces <= 19 chars) - pushed 16 ("2006-01-02 15:04") - size 9 (right-aligned for trailing-unit readability) - status variable suffix (no later column to misalign) A new `fitColumn(s, width)` helper does the pad-or-truncate work. Width is measured in runes (not bytes), so a multi-byte tag character doesn't break alignment. Tests - `TestECRImageDisplayTitleColumnsAlign`: three rows with short / medium / long tags all place the digest substring at the same rune position. Long tag truncates with the ellipsis marker. - `TestFitColumn`: 6 sub-cases pinning the helper's contract (pad / exact / truncate / unicode / width=0 / width=1 overflow). Verified locally: `go test ./...` → all 10 packages pass. Signed-off-by: SAY-5 <say.apm35@gmail.com>
|
/q review |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully addresses issue #195 by implementing fixed-width column formatting for the ECR image list, making it easier to scan when tags have varying lengths. The solution is well-designed with proper handling of edge cases and comprehensive test coverage.
Critical Change Requested
- Documentation error in
fitColumnfunction comment that misrepresents the padding behavior (identified above)
Implementation Strengths
The fix includes solid engineering practices: column width constants, rune-aware string handling for proper Unicode support, comprehensive test coverage (alignment verification and helper function tests), and proper edge case handling in fitColumn (width <= 0, exact match, overflow, width=1).
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| // fitColumn left-pads `s` to exactly `width` runes so subsequent columns | ||
| // align in the rendered list. Long values are truncated with a single | ||
| // trailing ellipsis. Width is measured in runes (not bytes) so multi-byte | ||
| // tag characters don't break the layout. |
There was a problem hiding this comment.
🛑 Documentation Error: Comment states "left-pads" but implementation right-pads (adds spaces to the right of the string). This will confuse future maintainers about the function's behavior.
| // fitColumn left-pads `s` to exactly `width` runes so subsequent columns | |
| // align in the rendered list. Long values are truncated with a single | |
| // trailing ellipsis. Width is measured in runes (not bytes) so multi-byte | |
| // tag characters don't break the layout. | |
| // fitColumn right-pads `s` to exactly `width` runes so subsequent columns | |
| // align in the rendered list. Long values are truncated with a single | |
| // trailing ellipsis. Width is measured in runes (not bytes) so multi-byte | |
| // tag characters don't break the layout. |
There was a problem hiding this comment.
@SAY-5 Can you fix above comment? :) Thank you for contribute.
YoungJinJung
left a comment
YoungJinJung
left a comment
There was a problem hiding this comment.
Please modify q developer bot review and merge this PR.
Signed-off-by: SAY-5 <say.apm35@gmail.com>
|
Fixed the doc comment in 349d1dc. |
|
Addressed in 349d1dc: comment now reads 'right-pads' per the suggestion. |
3 participants
Closes #195.
What
ECRImage.DisplayTitlerendered tag / digest / pushed / size / statusas a space-padded but variable-width string. When tags had different
lengths the digest column started at a different visual position per
row, making the list hard to scan.
Fix
Pin each column to a fixed rune-width and pad/truncate accordingly:
…on overflowshortDigestalready produces ≤ 19 chars2006-01-02 15:04New
fitColumn(s, width)helper does the pad/truncate. Width ismeasured in runes, not bytes, so a multi-byte tag character
doesn't break alignment.
Acceptance criteria:
….free-form).
Tests
internal/services/aws/ecr_model_test.go(new):TestECRImageDisplayTitleColumnsAlign— three rows with short /medium / long tags all place the digest substring at the same
rune position. Long tag truncates with the ellipsis marker.
(Bytes can disagree because
…is 3 UTF-8 bytes / 1 rune; thetest compares rune positions because that's what aligns visually
in a terminal.)
TestFitColumn— 6 sub-cases pinning the helper's contract(pad / exact / truncate / unicode / width=0 / width=1 overflow).
Verification
go build ./...✅go test ./...→ all 10 packages pass.