RE: Reddit feedback request

[frankhaugen]

Greetings, I decided to make my feedback here instead of comments, so you can ref it in other issues, or PRs

Reddit thread: 🌟 PowerPipe: An open-source .NET library for constructing workflows with a fluent interface

First of all: Great job on making something and putting it out for others to use and critique, it can be a scary thing to do, so kudos for doing it. I going to be a bit nitpicky, so though it might seem negative when I write "you should do X not Y", it's just some random dude's opinion on the internet,, please don't be offended, because the over-all here is that you have done a good job and you are on the right track to do a GREAT job 👍

Quick Suggestions

• Use Directory.Build.props -file to standardize common things between csproj -files
• Multi-target frameworks net6.0, net7.0, and netstandard2.1 (and in a couple of months net8.0)
• The PowerPipe.Extensions.MicrosoftDependencyInjection is fine for the package name, but should probably not be the namespace for the extension methods
• Split your samples into named scenarios that solves some common scenario and name the steps with names, (can be numbered, but instead of Step1, Step2, name them Step1Initialize, Step2Convert)
• Do more with custom exceptions, either multiple types, or add properties to it to make it more informative
• Add ILogger and use it vigorusly with good use of logging levels
• Update readme -examples to conform to requirements
• OnError() that takes a IPipelineStep would be nice
• PipelineContext should have a "log" mechanism to register things similar to ILogger<>, but is more intended for "user consumption"
• Use nullable reference types
• IPipeline<> and IPipelineStep<> shouldn't be buried in a namespace, make them easier to find by placing them in the "root" namespace

Where's the XML-docs?

XML-docs like these are essential to help you customers use your library correctly:

    /// <summary>
    /// Gets a random direction vector.
    /// </summary>
    /// <param name="random"></param>
    /// <returns>A normalized Vector3</returns>
    public static Vector3 NextDirection(this Random random)
    {
        // removed for brevity
    }

You need to add <GenerateDocumentationFile>true</GenerateDocumentationFile> to your csprojs PropertyGroup for the XML-docs to be compiled into the package

InternalsVisibleTo should be defined in csproj

You have this class AssemblyInfo.cs

using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("PowerPipe.UnitTests")]

you should instead add this to your csproj:

    <ItemGroup>
        <InternalsVisibleTo Include="$(AssemblyName).Tests"/> <!-- This is a neat little trick to make it work in a Directory.Build.props -file -->
        <InternalsVisibleTo Include="PowerPipe.Tests"/>
    </ItemGroup>

Add an extension method to DI extensions with Action

To make it easier to register something, try to find a way to get this pattern to work for your DI extension methods, would be very nice to just do this:

    services.AddPowerPipe<TestPipelineContext, TestPipelineResult>(builder => builder
        .Add<TestStep1>()
        .AddIf<TestStep1>(_ => true)
        .AddIfElse<TestStep1, TestStep2>(_ => true)
    );

This might also expose some weaknesses in your DI strategy, because I see some major hurdles to use this comfortably

Steps shouldn't be responsible for calling next step, and they shouldn't be aware of each other

I'm not a fan of how a step calls the next step (from sample):

    public IPipelineStep<SamplePipelineContext> NextStep { get; set; }
    public async Task ExecuteAsync(SamplePipelineContext context, CancellationToken cancellationToken)
    {
        Console.WriteLine($"{nameof(SampleStep3)} Executed");
        await NextStep.ExecuteAsync(context, cancellationToken);
    }

I've made a pipeline engine or two, and they have been "returns or throws" to decide what is happening next, because if there is a lot of data involved, I want to dispose anything in a class and only keep what I need, but your design holds everything in a hot path, so if I have hundreds of steps all having some data in them, they will eat memory like crazy.

Input/Output data in steps

I have a hard time getting a feel for how you are intending data-transfer between steps, because you aren't allowing some intuitive mechanism for data transfere, only thing I found was doing this, which is a bit jank, if the data has to live through the entire pipeline only to be transported between two steps:

	public class FetchStep : IPipelineStep<MyDataContext>
	{
		public IPipelineStep<MyDataContext> NextStep { get; set; }

		public async Task ExecuteAsync(MyDataContext context, CancellationToken cancellationToken)
		{
			context.FetchStepOutput.Add("Bob", 24);
		}
	}

	public class PushStep : IPipelineStep<MyDataContext>
	{
		public IPipelineStep<MyDataContext> NextStep { get; set; }

		public async Task ExecuteAsync(MyDataContext context, CancellationToken cancellationToken)
		{
		}
	}

	public class MyDataContext : PipelineContext<MyData>
	{
		public Dictionary<string, int> FetchStepOutput { get; set; } = new Dictionary<string, int>();
		
		public List<string> PushStepOutput { get; set; } = new List<string>();
		
		public override MyData GetPipelineResult()
		{
			return new MyData();
		}
	}

	public class MyData
	{
		public string Name { get; set; }
		public int Age { get; set; }
	}

---

My simple implementation that I made that I based most of my feedback

using PowerPipe;
using PowerPipe.Builder;
using PowerPipe.Extensions.MicrosoftDependencyInjection;
using PowerPipe.Interfaces;

var services = new ServiceCollection();

services.AddPowerPipe();
services.AddPowerPipeStep<FetchStep, MyDataContext>();
services.AddPowerPipeStep<PushStep, MyDataContext>();
services.AddSingleton<IPipeline<Person>>(x => new PersonPipeline(x.GetRequiredService<IPipelineStepFactory>()).Build());

var serviceProvider = services.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true});

var pipeline = serviceProvider.GetRequiredService<IPipeline<Person>>();

await pipeline.RunAsync();


public class PersonPipeline
{
	private readonly IPipelineStepFactory _pipelineStepFactory;

	public PersonPipeline(IPipelineStepFactory pipelineStepFactory)
	{
		_pipelineStepFactory = pipelineStepFactory;
	}

	public IPipeline<Person> Build()
	{
		var context = new MyDataContext();
		var builder = new PipelineBuilder<MyDataContext, Person>(_pipelineStepFactory, context);
		
		builder
			.Add<FetchStep>()
			.Add<PushStep>();
			
		return builder.Build();
	}
}

public class FetchStep : IPipelineStep<MyDataContext>
{
	public IPipelineStep<MyDataContext> NextStep { get; set; }

	public async Task ExecuteAsync(MyDataContext context, CancellationToken cancellationToken)
	{
		context.People.Add("Bob", 24);
		context.People.Add("Lisa", 21);
		Console.WriteLine("Fetched");
		await NextStep.ExecuteAsync(context, cancellationToken);
	}
}

public class PushStep : IPipelineStep<MyDataContext>
{
	public IPipelineStep<MyDataContext> NextStep { get; set; }

	public async Task ExecuteAsync(MyDataContext context, CancellationToken cancellationToken)
	{
		Console.WriteLine("Pushed");
		await NextStep.ExecuteAsync(context, cancellationToken);
	}
}

public class MyDataContext : PipelineContext<Person>
{
	public Dictionary<string, int> People { get; } = new Dictionary<string, int>();
	
	public override Person GetPipelineResult()
	{
		var person = People.First();
		return new Person()
		{
			Name = person.Key,
			Age = person.Value
		};
	}
}

public class Person
{
	public string Name { get; set; }
	public int Age { get; set; }
}

[mvSapphire]

Hi,

Thank you for providing feedback, this is incredibly helpful!

I welcome constructive criticism, here are my thoughts and responses to your suggestions:

• Directory.Build.props: I'll explore using Directory.Build.props. This is a great suggestion.

• Multi-target frameworks: It's on my to-do list, thanks.

• Namespace for extension methods: You are right, better to use Microsoft.Extensions.DependencyInjection namespace.

• Samples and scenarios: It's a good idea to create more specific samples.

• Custom exceptions: I'll consider adding more properties to exceptions but IMO inner exception stack trace it's enough.

• Logging with ILogger: Already on my to-do list.

• Readme examples: Currently I'm working on GitHub pages documentation, I'll add more wider description of the project, and features and add more examples.

• OnError() method: I'll consider adding an OnError(). This can be a valuable addition.

• Nullable reference types: Not a fan of it. But I'll take a closer look.

• Namespace placement for IPipeline<> and IPipelineStep<>: I agree that these interfaces should be more accessible.

• XML documentation: I apologize for missing the XML documentation. I'll add it ASAP.

• InternalsVisibleTo: I'll replace the AssemblyInfo.cs with the .

• DI extension method with Action: I'll explore ways to make it easier to register services using a pattern similar to the one you've proposed. It might be useful for small services.

• Steps not responsible for calling next step: Yeah maybe you are right that the return/throw mechanism might be easier. But I'll keep NextStep, we already use it for enterprise projects so I don't want to introduce breaking changes (or changes that require a lot of changes). And for now, we don't face any memory problems

• Input/Output data in steps: I'll consider options for improving the mechanism of data transfer between steps.

Once again, thank you for your valuable input!