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

Support Add Task Activities from Abstract or Non-Sealed Class #756

Merged
merged 6 commits into from
Jul 12, 2022

Conversation

elokaac
Copy link
Collaborator

@elokaac elokaac commented Jun 24, 2022

This change extends TaskHubWorker.AddTaskActivitiesFromInterface by supporting adding activities from abstract or non-sealed classes.

Copy link
Collaborator

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

I worry a little bit about this getting messy since methods on classes aren't always going to be properly designed to handle this kind of "remoting". It seems like an area where developers could run into various issues accidentally.

Would you be willing to consider a design where classes must opt certain methods into being used as interfaces, perhaps using an attribute? For example:

public abstract class ClassClient
{
    [TaskMethod]
    public abstract Task<T[]> Run<T>();
}

@elokaac
Copy link
Collaborator Author

elokaac commented Jun 25, 2022

I worry a little bit about this getting messy since methods on classes aren't always going to be properly designed to handle this kind of "remoting". It seems like an area where developers could run into various issues accidentally.

Would you be willing to consider a design where classes must opt certain methods into being used as interfaces, perhaps using an attribute? For example:

public abstract class ClassClient
{
    [TaskMethod]
    public abstract Task<T[]> Run<T>();
}

Having developers opt in methods when designing a class makes sense, though I feel like it makes things not so transparent for a user of the class who expects to be able to invoke all available methods.

@elokaac
Copy link
Collaborator Author

elokaac commented Jun 28, 2022

@cgillum , just curious, when you say "methods on classes aren't always going to be properly designed to handle this kind of remoting", what do you mean?

What differentiates class methods from interface methods in this context? Asides from the possibility of classes having private, protected or static methods?

@elokaac
Copy link
Collaborator Author

elokaac commented Jul 12, 2022

@cgillum , just curious, when you say "methods on classes aren't always going to be properly designed to handle this kind of remoting", what do you mean?

What differentiates class methods from interface methods in this context? Asides from the possibility of classes having private, protected or static methods?

@cgillum , just checking in on this if you don't mind sharing your thoughts.

@cgillum
Copy link
Collaborator

cgillum commented Jul 12, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 12, 2022

EDIT: pipeline started successfully.

Copy link
Collaborator

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

LGTM!

@elokaac
Copy link
Collaborator Author

elokaac commented Jul 12, 2022

LGTM!

Thanks @cgillum. Can you help me merge it? Or how can I get access to be able to merge pull requests I create?

@cgillum cgillum merged commit a8697b3 into Azure:main Jul 12, 2022
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.

2 participants