Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make spawn builder harder to misuse #83

Merged
merged 5 commits into from
May 22, 2024
Merged

Make spawn builder harder to misuse #83

merged 5 commits into from
May 22, 2024

Conversation

mbernat
Copy link
Contributor

@mbernat mbernat commented May 20, 2024

This is a follow-up to #82 to make it a hard error when the user forgets to provide an address to an actor.

This breaks the API because now one has to call .with_default_capacity() if they really don't want to provide or configure the address.

Please let me know how to navigate the upgrade process. I only bumped the Cargo version but we'll also have to publish this, possibly prepare a changelog, etc.

@mbernat mbernat requested review from strohel and PabloMansanet May 20, 2024 02:10
Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Nice! I have just some little docstrings suggestions.

Can you please pre-test this by using this branch in portal, and opening the required changes as a draft PR? To see the effect. Or maybe this doesn't require any portal changes?

Please let me know how to navigate the upgrade process. I only bumped the Cargo version but we'll also have to publish this, possibly prepare a changelog, etc.

I usually just use cargo-release. Let's pair on releasing the version once this is merged. Bumping the version in this PR is correct IMO (otherwise we could forget this is semver-breaking change).

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@mbernat
Copy link
Contributor Author

mbernat commented May 21, 2024

Nice! I have just some little docstrings suggestions.

Thanks, looking good, I applied all the suggestions.

Can you please pre-test this by using this branch in portal, and opening the required changes as a draft PR? To see the effect. Or maybe this doesn't require any portal changes?

It does require changes, here's the PR: https://github.com/tonarino/portal/pull/3333

I usually just use cargo-release. Let's pair on releasing the version once this is merged. Bumping the version in this PR is correct IMO (otherwise we could forget this is semver-breaking change).

Nice, let's do that.

Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

LGTM! The changes make sense to me, I just have a small comment on one of the examples.

@@ -4,7 +4,7 @@ use std::time::{Duration, Instant};
use tonari_actor::{Actor, Context, Event, Recipient, System};

#[derive(Debug, Clone)]
struct StringEvent(String);
struct StringEvent(());
Copy link
Member

Choose a reason for hiding this comment

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

Was this change due to a clippy warning? If so and we just want a unit struct, then maybe just struct ActorEvent; is enough. A tuple struct with just the unit type seems a bit strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's because of clippy. Will fix.

Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

LGTM! Just one unrelated change request for the fix clippy part. Conditionally approving already.

src/lib.rs Outdated Show resolved Hide resolved
@mbernat mbernat merged commit 4ce67b5 into main May 22, 2024
6 checks passed
@mbernat mbernat deleted the safer-spawn-builder branch May 22, 2024 02:44
@mbernat
Copy link
Contributor Author

mbernat commented May 31, 2024

@strohel Please let me know when you can spare a minute to release this with me, thanks!

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