add read tool formatters#1016
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
LasmarKhalifa
force-pushed
the
06-30/pi-tool-formatter-read
branch
from
af63b1e to
1522547
Compare
LasmarKhalifa
force-pushed
the
06-30/pi-tool-formatter-bash
branch
from
d2b7c43 to
3ee55e7
Compare
LasmarKhalifa
force-pushed
the
06-30/pi-tool-formatter-read
branch
from
1522547 to
2162423
Compare
LasmarKhalifa
requested review from
dersam,
juniper-shopify,
mathiusj and
nfgrep
as code owners
mathiusj
left a comment
mathiusj
left a comment
There was a problem hiding this comment.
read formatters look good. Two edge/consistency notes.
| path = truncate(path) | ||
| details = if limit | ||
| offset ||= 1 | ||
| "lines #{offset}–#{offset + limit - 1}" |
There was a problem hiding this comment.
Edge case: with limit: 0 this renders (lines 1–0) — a backwards range (offset + limit - 1 = 1 + 0 - 1 = 0). limit: 0 isn't obviously invalid input from Pi. Minor, but a limit.positive? guard (or documenting that limit is assumed ≥1) avoids the inverted range.
| # READ OK 0 lines | ||
| # | ||
| #: () -> String | ||
| def format_read |
There was a problem hiding this comment.
Counting inconsistency with the later sibling formatters. This counts content.to_s.lines.length (includes blank lines). The find/ls result formatters landing later in the stack use .map(&:strip).reject(&:empty?).length (drop blanks), so content "a\n\nb" reports 3 here vs 2 there. Same "count the lines" job, two rules. Worth making deliberate now — either align, or add a comment that read reports file line count (blanks included) while entry-listing tools don't.
2 participants
Closes #1024