Skip to content

Consider using Action<InvocationContext> and removing IInvocationResult #1936

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

Closed
Tracked by #1891
jonsequitur opened this issue Nov 2, 2022 · 0 comments · Fixed by #1948
Closed
Tracked by #1891

Consider using Action<InvocationContext> and removing IInvocationResult #1936

jonsequitur opened this issue Nov 2, 2022 · 0 comments · Fixed by #1948
Assignees
Labels
Area-API needs discussion Further discussion required
Milestone

Comments

@jonsequitur
Copy link
Contributor

jonsequitur commented Nov 2, 2022

    // Usual "classes over interfaces" comment.
    // Definitely do not make a public interface that the library doesn't already provide at least three different implementations of (non-public ones count), otherwise we don't have confidence in the shape.
    public interface IInvocationResult 
    {
        void Apply(InvocationContext context);
    }
 
        // Where does this cancellation token come from?
        public System.Threading.CancellationToken GetCancellationToken();
    }
 
    // Even with a renamed concept of "middleware", I'm not sure a delegate feels like the right thing.
    // And what should a middleware do with "next"?  (Was the return type of the delegate supposed to be Task?)
    // I'm afraid this whole thing needs a full redesign (or, preferably, to be cut).
    public delegate void InvocationMiddleware(InvocationContext context, Func<InvocationContext,Task> next);
    
    // You are aware that callers can pass any value between int.MinValue and int.MaxValue, inclusive, right?
    public enum MiddlewareOrder
    {
        Default = 0,
        ErrorReporting = 1000,
        ExceptionHandler = -2000,
        Configuration = -1000,
    }
}
 
@jonsequitur jonsequitur mentioned this issue Nov 2, 2022
56 tasks
@jonsequitur jonsequitur changed the title Consider using Action<InvocationContext> instead of an interface Consider using Action<InvocationContext> and removing IInvocationResult Nov 2, 2022
@jonsequitur jonsequitur added needs discussion Further discussion required Area-API labels Nov 2, 2022
adamsitnik added a commit to adamsitnik/command-line-api that referenced this issue Nov 4, 2022
@adamsitnik adamsitnik self-assigned this Nov 4, 2022
@jonsequitur jonsequitur added this to the 2.0 GA milestone Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-API needs discussion Further discussion required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants