Skip to content

Conversation

@str4d
Copy link
Collaborator

@str4d str4d commented Mar 12, 2025

Stacked on #34.


[patch.crates-io]
abscissa_core = { git = "https://github.com/str4d/abscissa.git", rev = "8da8e10cad02aa4a93099cb4942b48fbdc54eb89" }
abscissa_tokio = { git = "https://github.com/str4d/abscissa.git", rev = "8da8e10cad02aa4a93099cb4942b48fbdc54eb89" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewed that PR.

@str4d str4d force-pushed the components-refactor branch from 49174de to 3542cbd Compare March 12, 2025 08:06

info!("Spawned Zallet tasks");

// ongoing tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

(Not specific to this PR.)

Does pin! have a side effect causing its argument to be pinned in the current scope, or is the pin dependent on holding the returned Pin<_>? From reading the docs for std::pin, Pin<_>, and the examples for the pin! macro, I thought it was the latter.

Copy link
Contributor

@daira daira Mar 13, 2025

Choose a reason for hiding this comment

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

Oh this is tokio::pin!, not std::pin::pin!. That is sufficiently confusing (and the semantics sufficiently different) that I would prefer to see an explicit tokio::pin!(...).


Ok::<_, Error>(())
})
.map_err(|e| FrameworkErrorKind::ComponentError.context(e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map_err(|e| FrameworkErrorKind::ComponentError.context(e))?;
.map_err(|e: Error| FrameworkErrorKind::ComponentError.context(e))?;

Ok::<(), Error>(())
})?;

Ok::<_, Error>(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ok::<_, Error>(())
Ok(())

Err(e) => Err(ErrorKind::Init.context(e)),
}?;

Ok::<(), Error>(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ok::<(), Error>(())
Ok(())

info!("Creating empty database");
}
let handle = self.handle().await?;
handle.with_mut(|mut db_data| {
Copy link
Contributor

@daira daira Mar 13, 2025

Choose a reason for hiding this comment

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

Suggested change
handle.with_mut(|mut db_data| {
handle.with_mut(|mut db_data| -> Result<_, Error> {

(Non-blocking refactoring, together with the 3 suggestions below.)

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK with non-blocking suggestions.

Base automatically changed from misc-fixes to main March 14, 2025 03:29
@str4d str4d merged commit 75c8e85 into main Mar 15, 2025
19 checks passed
@str4d str4d deleted the components-refactor branch March 15, 2025 02:44
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