Skip to content

Adds a Set overload which takes a lambda factory method #36

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

Merged
merged 5 commits into from
May 21, 2015

Conversation

Quantumplation
Copy link
Contributor

  • The specific use case for this is that we wanted to be able
    to use equivalence classes. However, if you just use them naively,
    it would generate a single value and use it for all of the instances
    it created.
  • The lambda overload not only enables this case, but gives some power
    to constructing incrementing counters and the like. For example,
    using a Queue to generate an ordered set of names, or other such use
    cases.

 - The specific use case for this is that we wanted to be able
   to use equivalence classes.  However, if you just use them naively,
   it would generate a single value and use it for all of the instances
   it created.
 - The lambda overload not only enables this case, but gives some power
   to constructing incrementing counters and the like.  For example,
   using a Queue to generate an ordered set of names, or other such use
   cases.
var builder = new CustomerBuilder()
.Set(x => x.FirstName, "Pi")
.Set(x => x.LastName, "Lanningham")
.Set(x => x.YearJoined, () => counter++);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wanted to provide an overload that would take a lambda Func from TObj to TValue, so that you could base your value on the partially constructed object (i.e. make sure that AmountPaid was less than AmountOwed, or something),but that would have required too much internal structuring, so I left it for another day.

Copy link
Member

Choose a reason for hiding this comment

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

It would be too hard to do the partially constructed object, you could do one that takes the builder so you can use a Get call though?

@Quantumplation
Copy link
Contributor Author

The more I think about this, I kind of want to replace the values collection entirely. i.e., remove _properties, and when you Set with an explicit value, we just store () => value in the _propFactories. This makes things simpler, and works pretty well for some other ideas I have (and will be submitting a pull request for soon)

What do you think?

@robdmoore
Copy link
Member

Agreed - I'd much prefer that. It's a much cleaner way of doing it. I'd still call it properties though rather than propFactories

@Quantumplation
Copy link
Contributor Author

My coworker suggested that the lambda take the the AutoValueFixture, so you can write

    .Set(x => x.YearJoined, any => any.PositiveInteger());

What do you think of that idea as well?

…hods

 - It's a lot cleaner to use the factory lambdas as just constant functions
   for the other Set method.
@robdmoore
Copy link
Member

Yeah, that does make sense. I wonder how that will affect our ability to also pass in a builder...

return Any.Get(type, propertyName);
return _properties[propertyName];
Func<object> factory;
if (_properties.TryGetValue(propertyName, out factory)) return factory();
Copy link
Member

Choose a reason for hiding this comment

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

Can this be on two lines to meet the coding conventions of this project please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Do you have the coding conventions documented somewhere? I looked for a .md file somewhere and didn't see one.

Copy link
Member

Choose a reason for hiding this comment

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

Not documented no - just based on the rest of the code ;)

e.g. see the if statement in the deleted lines just above this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd recommend starting a document with this case, and add to it in the future. It's hard to pick up on what the coding style rules of a project are just from the rest of the code, especially for a new contributor. If you want, I'll include it in my pull request.

@Quantumplation
Copy link
Contributor Author

What do you mean also pass in a builder? Did you mean, a builder for the second parameter? because that's as simple as

.Set(x => x.Roommate, _ => studentBuilder.Build());

@robdmoore
Copy link
Member

I more meant what we were talking about above about passing the builder instance (where you mentioned passing in a "partially constructed object") into the lambda

@Quantumplation
Copy link
Contributor Author

Ah, right. I'll leave that in your hands to decide which you think would be more useful. We can always create an AutoValueFixture and use it via closure for that use case, but we'd never be able to get the partial values in there without the parameter (Ah, I see now, I missed one of your comments somehow). Then again we could have various overloads and internally layer another lambda over them which discards the second parameter.

 - This will help new contributors get started, and keep them from
   stepping on anyone's stylistic toes.
 - Expand as you see fit!
robdmoore added a commit that referenced this pull request May 21, 2015
Adds a Set overload which takes a lambda factory method
@robdmoore robdmoore merged commit 67df72a into TestStack:master May 21, 2015
@robdmoore
Copy link
Member

Thanks for this

@Quantumplation
Copy link
Contributor Author

That's cool about the CONTRIBUTING.MD, I didn't know that! Awesome! And no worries! I plan on submitting another pull request in the next few days, but I'm going to open an issue first to let you provide your input on it.

@robdmoore
Copy link
Member

FYI there are a couple of minor things I need to look at then I'll push to NuGet.

I'm doing a presentation at a virtual conference about this library next week so it will be then by the latest, but I hope to do it before then.

@Quantumplation
Copy link
Contributor Author

Oh awesome, what conference?

@robdmoore
Copy link
Member

@robdmoore
Copy link
Member

Thanks again for this - I've released it as v3.1.0

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