nickdodd79 / AutoBogus

A C# library complementing the Bogus generator by adding auto creation and population capabilities.
MIT License
431 stars 50 forks source link

Can't have a Bogus FinishWith configured #24

Closed akamud closed 4 years ago

akamud commented 4 years ago

Since Bogus allows only a single FinishWith method, the way AutoBogus works its magic makes it impossible for someone to configure a FinishWith for a Type like this:

public class PersonBuilder : AutoFaker<Person>
{
//    ... props
}

// ...
var personBuilder = new PersonBuilder().FinishWith((f, p) => 
{ 
    // something 
})
.Generate();

This is common in a scenario where a property doesn't have a setter and has to be configured through methods. So something like this:

public class PersonBuilder : AutoFaker<Person>
{
    public PersonBuilder WithAddresses(int count = 1)
    {
        // store how many addresses must be generated
    }

    public PersonBuilder WithEmails(int count = 1)
    {
        // store how many emails must be generated
    }

    // other methods for Collections without setter

   public override Generate(string ruleSets = null)
   {
        FinishWith((f, p) =>
        {
            // Calls methods that populate Addresses, Emails, etc.
        });
   }
}

From what I looked at the code, the culprit is this line.

I see a few ways this could be fixed: Bogus could allow multiple finalizers (I don't see why this isn't allowed anyway), or AutoBogus could execute that code in a different way.

What do you think? Is this something that should be improved?

nickdodd79 commented 4 years ago

Hey @akamud

Yes, the FinishWith() handler is hijacked to populate the empty properties once the Faker Generate() method has done it's thing. I think there are a couple of options to explore:

  1. AutoBogus doesn't hijack the FinishWith() and instead invokes the finishing logic separately after the internal Generate() is called. I would also remove the use of CreateActions() and invoke that separately.
  2. Bogus extends the FinishWith() to allow for multiple handlers. There are also create handlers that could possibly be changed to allow multiples.
  3. Bogus changes access to FinalizeActions field so it is not internal. As with 2, this could probably be inclusive of the creation handlers. We can then extract the configured finish handler and call it from within the AutoBogus finish handler.

I think some insight from @bchavez would be helpful in deciding a path (as i'm not sure of the impact for 2 and 3). However, I am inclined to think that 1 is the best approach, as there is no strict need to use the CreateActions() and FinishWith() methods.

Nick.

nickdodd79 commented 4 years ago

@akamud

I have decided to go with option 1 as some code review has shown room for improvement in that area. I will get on this ASAP and let you know when a new version is available.

Nick.

bchavez commented 4 years ago

Hey @nickdodd79 @akamud,

I really apologize for the delay. I've been busy preparing for job interviews at a few places. Also been thinking a lot about moving Bogus to the .NET Foundation as part of their new Project Maturity Model announced early this week at .NET Conf; mostly as a move to gain more users and as a way of establishing confidence for bigger businesses. @nickdodd79 , if you'd like to be a part of it and you think it's right for you, please send me an email and we can talk more about it. Essentially, PML Level 3 and 4 require more than 1 maintainer on the project.

As for Bogus having multiple .FinishWith methods, I'm not sure. The conceptional API usage for Faker<T> is that the last-call wins. IE:

The same conceptual API consistency applies to .FinishWith( f, t =>); last-call wins.

At least from Bogus' point of view; I'm not sure how helpful keeping track of past rules is important to Faker<T> other than in this specific case here.

I like method 3 that you mentioned as a possible workaround for this issue is for AutoBogus to store a reference to any finalizers being kept track in Faker<T> and call them after AutoBogus is done hooking FinishWith. For sure, I can help with this by opening up some kind of accessor to a member if anything is private. But it seems like we already have that protected accessor set: https://github.com/bchavez/Bogus/blob/master/Source/Bogus/Faker%5BT%5D.cs#L54

PR if you need anything more opened if it could help your implementation.

Lastly, another possibility if we want to get crazy hard-core about it; any Action is really a delegate under the hood; specifically, a multi-cast delegate? So, as a crazy C# hack, you technically can combine two Action methods in C#; like this check it out:

void Main()
{
   Action a = () => "Hello A".Dump();
   Action b = () => "Hello B".Dump();
   Action c = a + b;
   c();
}
Hello A
Hello B

IIRC, under the hood, the C# compiler will use Delegate.Combine(a,b) to produce c. Thereby hacking your way around this neat little problem of needing to keep track of a 'list' of invocations. :smiling_imp: Specifcally, get the FinalizeAction.Action and attach your custom hook ... might work?

Let me know your thoughts.

Thanks, Brian

:mount_fuji: :man_playing_water_polo: Creedence Clearwater Revival - Green River

nickdodd79 commented 4 years ago

Hey @bchavez

Thanks for your input. Having reviewed the AutoBogus code, I have decided the best path is to not use the Bogus handlers and invoke any finalize actions separately. There are some use cases, where this approach would be better and also simplifies the changes needed.

Thanks for the invite to join the project - I will fire over an email.

Nick.

nickdodd79 commented 4 years ago

Hey @akamud

Apologies for the delay in getting a fix out for this. There have been several events, commitments and deadlines I have had that have stolen my time. I was hoping to have a new v3 out that made changes around the FinishWith, but finding enough time to make traction on it has been hard. I have some spare time over the next few weeks, so I am hoping to update v2 to as a quick fix.

Thanks for your patience. Nick.

metareven commented 4 years ago

Have you had any progress with this? Just refactored an entire project to use FinishWith to make some things smoother just to find out that this functionality is not supported with autobogus. Would be nice to know if I could keep my branch for later or if I should just go back to not using FinishWIth

nickdodd79 commented 4 years ago

Hey @metareven

Thanks for reminding me of this one. I had made the changes a while ago, just hadn't release it.

This capability has now been included in v2.8.

Nick.