Skip to content

operation: convert parents to async function#9140

Open
bnjmnt4n wants to merge 1 commit intomainfrom
bnjmnt4n/push-vvzoxzoklzkp
Open

operation: convert parents to async function#9140
bnjmnt4n wants to merge 1 commit intomainfrom
bnjmnt4n/push-vvzoxzoklzkp

Conversation

@bnjmnt4n
Copy link
Member

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added/updated tests to cover my changes
  • I fully understand the code that I am submitting (what it does,
    how it works, how it's organized), including any code drafted by an LLM.
  • For any prose generated by an LLM, I have proof-read and copy-edited with
    an eye towards deleting anything that is irrelevant, clarifying anything
    that is confusing, and adding details that are relevant. This includes,
    for example, commit descriptions, PR descriptions, and code comments.

@bnjmnt4n bnjmnt4n requested a review from a team as a code owner March 18, 2026 11:14
let mut op_to_restore = match op_to_redo.parents().at_most_one().ok().flatten() {
let mut op_to_restore = match op_to_redo
.parents()
.collect::<Vec<_>>()
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we use try_collect() to simplify a bit?

Copy link
Contributor

Choose a reason for hiding this comment

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

or make op.parents() collect into Vec<_>? Commit::parents() is implemented that way, and I don't have strong feeling that these parents() calls should be optimized.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a version which makes op.parents() return OpStoreResult<Vec<Operation>>, but it does make some of the other call sites have to translate those back to Vec<OpStoreResult<Operation>>. I’m not sure I prefer that.

Copy link
Contributor

@yuja yuja Mar 20, 2026

Choose a reason for hiding this comment

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

I think we'll need to redesign DAG utility anyway for a better async support. The current callback-based sync API already has redundant .collect() because a borrowed iterator can't be returned, and async fn would increase complexity of borrowing further.

let mut op_to_restore = match op_to_undo.parents().at_most_one() {
let mut op_to_restore = match op_to_undo
.parents()
.collect::<Vec<_>>()
Copy link
Member

Choose a reason for hiding this comment

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

same here

@bnjmnt4n bnjmnt4n force-pushed the bnjmnt4n/push-vvzoxzoklzkp branch from 7dd541a to 84a317a Compare March 19, 2026 16:04
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Since this PR seems to update all callers of closest_common_node_ok() to want Result<Vec<T>, E> instead of Vec<Result<T, E>, perhaps we should update closest_common_node_ok() to work with that type.

} else {
let op = resolve_op(&args.operation).await?;
let parent_ops: Vec<_> = op.parents().try_collect()?;
let parent_ops: Vec<_> = op.parents().await?;
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can remove the type annotation. Same in many other places.

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.

3 participants