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

Facility to provide interface while registering actors #1350

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

siri-varma
Copy link

@siri-varma siri-varma commented Sep 24, 2024

Description

Added facility to provide Interface types while registering Actors.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1341

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@siri-varma siri-varma requested review from a team as code owners September 24, 2024 02:28
Signed-off-by: Siri Varma Vegiraju <svegiraju@microsoft.com>
Signed-off-by: Siri Varma Vegiraju <svegiraju@microsoft.com>
Signed-off-by: Siri Varma Vegiraju <svegiraju@microsoft.com>
Signed-off-by: Siri Varma Vegiraju <svegiraju@microsoft.com>
@siri-varma siri-varma force-pushed the users/svegiraju/pull-request-123 branch from 004c240 to c463186 Compare September 24, 2024 02:30
@siri-varma
Copy link
Author

@philliphoff @cgillum Would you folks be able to help with approving the workflows please ?

@WhitWaldo
Copy link
Contributor

@siri-varma All in all, I think this looks fine, but I think it'd be helpful to have documentation calling this out as it might be unclear to someone why they're experiencing an exception when implementing an interface that doesn't itself ultimately inherit from an IActor parent.

Otherwise, I think all the code looks fine. If you could write up a few words about it in this section, I'd be happy to accept this.

WhitWaldo and others added 2 commits October 8, 2024 11:48
@siri-varma
Copy link
Author

siri-varma commented Oct 10, 2024

@WhitWaldo I updated the md file. Let me know if it looks good. Thanks

@WhitWaldo
Copy link
Contributor

This will also close #694

Comment on lines +156 to +161
> [!IMPORTANT]
> When registering actors, note the return type requirements for the methods inside the interfaces:
> * Pattern 1: `options.Actors.RegisterActor<MyActor>()`
> * In this case, all interfaces implemented by `MyActor` must have methods that return only `Task` or `Task<T>`. This applies to every method in all interfaces `MyActor` implements.
> * Pattern 2: `options.Actors.RegisterActor<IMyActor, MyActor>()`
> * Here, only the `IMyActor` interface is required to have methods that return `Task` or `Task<T>`. Other interfaces `MyActor` are not subject to this restriction.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we force the use of IActor interface we can add a Diagnostics to block the compilation when the derived class contains methods that doesn't return Task<T>. Similar to what is done in the actors' source generator for the CancellationToken (example)

I usually don't like to find out particularities from documentation, if possible I prefer my editor to guide me.

What do you think about it?

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

Successfully merging this pull request may close these issues.

Allow to define which interfaces are important for the dapr actor host
4 participants