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

Adding ISpecimenBuilder as param on With method #1444

Conversation

luccanog
Copy link

Description

Accepting ISpecimentBuilder interface as value generator for With method:

  ISpecimenBuilder specimenBuilder = new ElementsBuilder<string>("ios", "windows", "linux");

  var foo = _fixture.Build<Data>()
                   .With(d => d.Name, specimenBuilder)
                   .Create();

Closed issues

#1421

Checklist

  • Reviewed the contribution guidelines
  • Linked the issue(s) the PR closes
  • Implemented automated tests and checked coverage
  • Provided inline documentation comments for new public API
  • Ran the full solution build and validation locally

@luccanog
Copy link
Author

@dotnet-policy-service agree

@luccanog
Copy link
Author

@aivascu The pipeline is failing due to previous .NET versions, and I've noticed recent PRs adding .NET 5 runtime like on #1443 being merged to the release branch.

May this PR be based on release/5.0.0 instead of the master branch? Couldn't find info about it on the code docs.

I was able to build, test, and pack it locally but the cmd script fails.

@aivascu
Copy link
Member

aivascu commented Feb 27, 2024

@luccanog the errors thrown by the pipeline are related to the SDK building the code. Looks like AppVeyor have updated their VS 2019 image. I'll add the errors to ignore list.

@aivascu
Copy link
Member

aivascu commented Feb 27, 2024

Looks like they outright removed .net core 1 and 2, and I've just finished re-balancing all versions on the release branch...
@luccanog could you rebase your PR on the release branch and target that instead of the master?

@luccanog luccanog changed the title Adding ISecimentBuilder as param on With method Adding ISpecimenBuilder as param on With method Feb 28, 2024
@luccanog luccanog changed the base branch from master to release/5.0.0 February 28, 2024 01:25
@luccanog
Copy link
Author

luccanog commented Feb 28, 2024

@aivascu The rebase is done.
I've noticed the Readme file was added up because it was modified on the master branch, please let me know if I should undo those changes or if I should keep them.

@aivascu
Copy link
Member

aivascu commented Feb 28, 2024

@luccanog yes please undo the readme. I'll synchronize them in a separate PR.

@luccanog luccanog changed the base branch from release/5.0.0 to master February 28, 2024 16:30
@luccanog luccanog changed the base branch from master to release/5.0.0 February 28, 2024 16:30
@luccanog
Copy link
Author

@aivascu I came back with the old content, but for some reason git hub still thinks there is a difference between files. It might be irrelevant to the process

@aivascu aivascu force-pushed the feature/composer-with-ispecimenbuilder branch from 6f796d5 to ad5889b Compare March 3, 2024 22:35
@aivascu
Copy link
Member

aivascu commented Mar 3, 2024

@luccanog I've rebased and squashed your commits on the release branch. I hope you don't mind.

I've had a brief look at your implementation, but I'm not done reviewing it for now.
I'm not convinced that piggybacking on the factory function overload is the way to go. As a workaround it might work fine, but there must be a more efficient way to compose the nodes than that.

Will let you know if I come up with anything.

@luccanog
Copy link
Author

luccanog commented Mar 4, 2024

@aivascu No problem at all, feel free to do any other rebase if needed.

About he piggybacking, do you mean the changes at the NodeComposer class? There I re-used an existent overload since the method has got what we need, but of course we have some other ways. As I see, we could call the WithCommand method directly at our new With method, and still use the same public BindingCommand signature.

In this way our new With method is going to be pretty similar to the existent one, but with a conversion from ISpecimenBuilder to a Func<TInput, TProperty>.

Please let me know when you got any thoughts.

@aivascu
Copy link
Member

aivascu commented Mar 4, 2024

@luccanog yes, I meant that you reused the builder syntax, to create a factory that's using the specimen builder. 😅
Sorry I should be more explicit.

Using the BindingCommand sounds better however I propose that we add a new overload for the public constructor.

public BindingCommand(Expression<Func<T, TProperty>> propertyPicker, ISpecimenBuilder builder)
{
    if (propertyPicker is null) throw new ArgumentNullException(nameof(propertyPicker));
    if (builder is null) throw new ArgumentNullException(nameof(builder));

    this.Member = propertyPicker.GetWritableMember().Member;
    this.ValueCreator = c => (TProperty)builder.Create(typeof(TProperty), c);
}

This would allow to pass the context directly into the builder and avoid going through several layers of abstractions.
I tried it locally and it seems to be working fine.

Let me know if you'll try these changes, and cover them with tests.

@luccanog
Copy link
Author

luccanog commented Mar 7, 2024

I just implemented the new BindindCommand conStructor and added more tests as requested. Seems to work properly.
Now we don't have to re-use the With method overload, and we can directly construct the BindingCommand with a ISpecimenBuilder

@aivascu
Copy link
Member

aivascu commented Mar 10, 2024

@luccanog after some consideration, I decided I don't want yet to couple BindingCommand to ISpecimenBuilder, so I reverted the changes and used the existing constructor.

@aivascu aivascu merged commit aa58120 into AutoFixture:release/5.0.0 Mar 10, 2024
3 checks passed
@luccanog
Copy link
Author

@aivascu alright. Thank you!

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.

None yet

2 participants