Skip to content

add edit tool formatters#1018

Open
LasmarKhalifa wants to merge 1 commit into
06-30/pi-tool-formatter-writefrom
07-02/pi-tool-formatter-edit
Open

add edit tool formatters#1018
LasmarKhalifa wants to merge 1 commit into
06-30/pi-tool-formatter-writefrom
07-02/pi-tool-formatter-edit

Conversation

@LasmarKhalifa

@LasmarKhalifa LasmarKhalifa commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Closes #1026

LasmarKhalifa commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@LasmarKhalifa LasmarKhalifa force-pushed the 06-30/pi-tool-formatter-write branch from f2399f0 to 4166e5d Compare July 3, 2026 15:30
@LasmarKhalifa LasmarKhalifa force-pushed the 07-02/pi-tool-formatter-edit branch from 7730ee2 to 676a57b Compare July 3, 2026 15:30
@LasmarKhalifa LasmarKhalifa marked this pull request as ready for review July 3, 2026 16:30

@mathiusj mathiusj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit formatters look good. Two small things.

#: () -> String
def format_edit
path = arguments[:path]
edits = arguments[:edits] || []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| [] only guards nil — if :edits ever arrives as a non-Array (e.g. a String or a single Hash), .length either silently returns the wrong count (char count for a String) or raises. Given this is display-only, I wouldn't add validation here; the clean fix is a rescue→format_unknown at the dispatch site in #1012 (commented there). Just flagging so it's a conscious choice rather than an assumption. Also note EDIT #{path} interpolates the path untruncated, so a long path blows past the one-line goal the comments state.

# EDIT OK
#
#: () -> String
def format_edit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readability nit: the result formatter reads @input[:path] while the call formatter (same field) reads arguments[:path]. Both correct — @input is the resolved call's args in the result context, arguments is the call's own ivar — but the naming divergence across the two files costs a beat to reconcile. A one-line comment noting they're deliberately different objects would help future readers.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants