Conversation
|
All contributors have signed the CLA ✍️ ✅ |
akshaynathan
changed the title
akshaynathan
force-pushed
the
draft-stream-responses-apply-patch-events
branch
from
ac74bdc to
aa91fe9
Compare
akshaynathan
changed the title
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
Needs an integration test. Search |
| return Err(e); | ||
| } | ||
| ParseMode::Lenient => check_patch_boundaries_lenient(&lines, e)?, | ||
| ParseMode::Streaming => check_patch_boundaries_streaming(&lines, e)?, |
There was a problem hiding this comment.
can we return the "actual patch scope" from this match expression? or individual check_ methods? that way we won't need the matches!(mode, ParseMode::Streaming below again.
|
|
||
| #[test] | ||
| fn test_parse_patch_streaming() { | ||
| assert_eq!( |
There was a problem hiding this comment.
can you please add a stress test that takes in one large patch that has types of operations, slices it in 1 character increments and feeds to the streaming parser. We should see the ever increasing hunk count. (feel free to add stricter assertions if you'd like)
| let active_path = hunks.last().map(Hunk::path).map(Path::to_path_buf); | ||
| let changes = convert_apply_patch_hunks_to_protocol(&hunks); | ||
| self.last_progress = Some(hunks); | ||
| Some(PatchApplyDeltaEvent { |
There was a problem hiding this comment.
this is not a delta event right? is the latest state.
PatchApplyUpdatedEvent?
| match event { | ||
| ResponseEvent::Created => {} | ||
| ResponseEvent::OutputItemDone(item) => { | ||
| if active_tool_argument_diff_consumer |
There was a problem hiding this comment.
I think we can clean this out unconditionally. you don't support parallel diff consumers anyway.
| tx.add_changed_paths(&[path("b"), path("c")]).await; | ||
| // Force a fresh throttle window so slow CI scheduling after the first | ||
| // receive cannot consume it before the blocked receive starts. | ||
| throttled.next_allowed = Some(Instant::now() + Duration::from_secs(1)); |
| PatchApplyBegin(PatchApplyBeginEvent), | ||
|
|
||
| /// Incremental model-generated structured changes for an `apply_patch` call. | ||
| PatchApplyDelta(PatchApplyDeltaEvent), |
There was a problem hiding this comment.
you also need to expose this event through app-server for App to be able to consume
There was a problem hiding this comment.
coming in the next PR
| /// Structured file changes parsed from the model-generated patch input so far. | ||
| pub changes: HashMap<PathBuf, FileChange>, | ||
| /// File path currently being written, when known. | ||
| pub active_path: Option<PathBuf>, |
There was a problem hiding this comment.
can this be calculated by the client based on changes[-1]?
There was a problem hiding this comment.
removed, will calculate on the client!
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
Very clean!! Nice work!
Let's add a core integration test and expose through app-server API. And maybe rename the event because it's not truly a delta
akshaynathan
force-pushed
the
draft-stream-responses-apply-patch-events
branch
3 times, most recently
from
ee0456a to
6effc01
Compare
…onses api. This is to enable clients to show progress during file writes. This does not work with apply_patch in function call mode, since that required adding streaming json parsing.
akshaynathan
force-pushed
the
draft-stream-responses-apply-patch-events
branch
from
1d185c2 to
4de8d61
Compare
2 participants
Adds new events for streaming apply_patch changes from responses api. This is to enable clients to show progress during file writes.
Caveat: This does not work with apply_patch in function call mode, since that required adding streaming json parsing.